-
Notifications
You must be signed in to change notification settings - Fork 159
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
Add assert in debug mode and tests #1416
Conversation
@@ -1,6 +1,9 @@ | |||
// Copyright 2019-2022 ChainSafe Systems | |||
// SPDX-License-Identifier: Apache-2.0, MIT | |||
|
|||
/// The maximum varint that can be serialized on 9 bytes. | |||
const MAX_VARINT: u64 = 2u64.pow(63)-1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about using usize
here? Unless we're supporting 32-bit systems it should be fine, plus we would get rid of some casts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. We indeed want 32-bit systems support like wasm32
. Protocol Labs had to port our code to explicitly use u64
(there are many commits, the one for Bitfield is here). We will probably use some of their crates instead of ours (I'm commiting here but this PR will have to be done on the ref-fvm
repo too to keep things in sync in the meantime).
Even if we don't want to support say 32-bit systems I think it's a good practice to use properly sized types (makes the code more readable and correct).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation! In this case, should we rather do an opposite cast which wouldn't be lossy? Given that potentially (on a 32-bit machine) MAX_VARINT as usize
would get truncated to 32 bits (u32::MAX
). In the end the behaviour would still be the same - on a 32 bit machine the condition will always pass regardless.
@@ -1,6 +1,9 @@ | |||
// Copyright 2019-2022 ChainSafe Systems | |||
// SPDX-License-Identifier: Apache-2.0, MIT | |||
|
|||
/// The maximum varint that can be serialized on 9 bytes. | |||
const MAX_VARINT: u64 = 2u64.pow(63)-1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation! In this case, should we rather do an opposite cast which wouldn't be lossy? Given that potentially (on a 32-bit machine) MAX_VARINT as usize
would get truncated to 32 bits (u32::MAX
). In the end the behaviour would still be the same - on a 32 bit machine the condition will always pass regardless.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #1361
Other information and links
Will notice PL so they can report PR on
ref-fvm
repo.