Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove unnecessary unsafe block #1515

Merged
merged 1 commit into from
Oct 31, 2022
Merged

remove unnecessary unsafe block #1515

merged 1 commit into from
Oct 31, 2022

Conversation

onur-ozkan
Copy link
Member

Signed-off-by: ozkanonur work@onurozkan.dev

@artemii235
Copy link
Member

@ozkanonur It was already done in this PR: #1514

@onur-ozkan
Copy link
Member Author

onur-ozkan commented Oct 28, 2022

@ozkanonur It was already done in this PR: #1514

Seems I couldn't pull it, sorry. Let me rebase the branch and only keep the unsafe commit. It's not required to be used there.

ps: That PR casts u64 to u32, that might cause problems which we talk in here

Signed-off-by: ozkanonur <work@onurozkan.dev>
@onur-ozkan onur-ozkan changed the title fix wasm compilation errors remove unnecessary unsafe block Oct 28, 2022
@sergeyboyko0791
Copy link

sergeyboyko0791 commented Oct 28, 2022

There shouldn't be problem because even if sleep_ms: u64 == u64::MAX, then it will be casted to sleep_ms as u32 == u32::MAX.

ps: That PR casts u64 to u32, that might cause problems which we talk in #1500

@onur-ozkan
Copy link
Member Author

onur-ozkan commented Oct 28, 2022

There shouldn't be problem because even if sleep_ms: u64 == u64::MAX, then it will be casted to sleep_ms as u32 == u32::MAX.

ps: That PR casts u64 to u32, that might cause problems which we talk in #1500

I guess we have a condition that checks if bigger than i32::MAX then in that function? Because thats very unpredictable scenario. It will not be always equal to max i32. It can be random number between 0-max u32 if there is no conditional checks.

@sergeyboyko0791
Copy link

Yes, that would be true if we casted u64 to i32, but we cast u64 to u32. Do I miss something?

I guess we have a condition that checks if bigger than i32::MAX then in that function? Because thats very unpredictable scenario. It will not be always equal to max i32. It can be random number between 0-max u32 if there is no conditional checks.

@onur-ozkan
Copy link
Member Author

Yes, that would be true if we casted u64 to i32, but we cast u64 to u32. Do I miss something?

I guess we have a condition that checks if bigger than i32::MAX then in that function? Because thats very unpredictable scenario. It will not be always equal to max i32. It can be random number between 0-max u32 if there is no conditional checks.

It's same for all castings from a type that supports bigger value to type support smaller value.

eg:

fn main() {
    // MAX value of unsigned 32 byte + 10
    let x: u64 = 4294967295 + 10;
    assert_eq!(x as u32, std::u32::MAX, "{} is not {}", x as u32, std::u32::MAX);
}

@sergeyboyko0791
Copy link

I was wrong... I thought I tested exactly the same case that you mentioned above, but it looks like I didn't...
Thank you for the clarification! Could you please fix it in this PR?
@ozkanonur

@onur-ozkan
Copy link
Member Author

onur-ozkan commented Oct 28, 2022

I was wrong... I thought I tested exactly the same case that you mentioned above, but it looks like I didn't... Thank you for the clarification! Could you please fix it in this PR? @ozkanonur

I will implement the safe-casting macros and update non-safe castings with using that macro. I will do it at weekend and make it r2r before monday morning. Today I have some debugging work on tendermint

Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@artemii235 artemii235 merged commit b27a9a4 into dev Oct 31, 2022
@artemii235 artemii235 deleted the fix-wasm-compatibility branch October 31, 2022 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants