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

Add assert in debug mode and tests #1416

Merged
merged 1 commit into from
Feb 10, 2022
Merged

Add assert in debug mode and tests #1416

merged 1 commit into from
Feb 10, 2022

Conversation

elmattic
Copy link
Contributor

@elmattic elmattic commented Feb 2, 2022

Summary of changes
Changes introduced in this pull request:

  • Add assert in debug mode to avoid misuses
  • Add two tests

Reference issue to close (if applicable)

Closes #1361

Other information and links

Will notice PL so they can report PR on ref-fvm repo.

@@ -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;
Copy link
Member

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.

Copy link
Contributor Author

@elmattic elmattic Feb 9, 2022

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).

Copy link
Member

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;
Copy link
Member

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.

@q9f q9f merged commit f96876c into main Feb 10, 2022
@q9f q9f deleted the elmattic/actors-review-B5 branch February 10, 2022 17:11
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.

B5: Forest bitfield rle writer will write 10-byte varints
3 participants