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

Use NonZeroUsize for constants #81

Merged
merged 2 commits into from
Jan 4, 2024
Merged

Use NonZeroUsize for constants #81

merged 2 commits into from
Jan 4, 2024

Conversation

cyphersnake
Copy link
Collaborator

By using NonZero we eliminate a class of errors where you don't get errors with zero length (e.g. traversing arrays with zero length just doesn't happen), however, no work is done and the data at the end is incorrect.

It also allows us to better reflex some places where we pass the size of some collection as a parameter and the collection turns out to be empty due to an error. Now at least we will get panic in such places

By using NonZero we eliminate a class of errors where you don't get errors with zero length (e.g. traversing arrays with zero length just doesn't happen), however, no work is done and the data at the end is incorrect
@cyphersnake cyphersnake requested a review from chaosma January 3, 2024 12:54
@cyphersnake cyphersnake self-assigned this Jan 3, 2024
Copy link
Collaborator

@chaosma chaosma left a comment

Choose a reason for hiding this comment

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

Can you describe the case where we silently fail if don't use NonZeroUsize? In the first synthesize phase, will this cause any error because it passes a potential zero size bit vector?

@cyphersnake
Copy link
Collaborator Author

I had exactly the same error when I was counting carry-bits. Because of a bug in the code, it always gave zero and it was not caught by tests or compiler, because it just calculated zero everywhere and traversed the collection zero times.

A bad variant of the same design is to put assert_ne!(field, 0); everywhere, but the NonZero variant is much more elegant

@cyphersnake cyphersnake requested a review from chaosma January 4, 2024 08:02
@chaosma
Copy link
Collaborator

chaosma commented Jan 4, 2024

I understand this approach can eliminate this specific type of error. But the use of NonZeroUsize and unsafe seems a little overkill. Let me think about if there is better approach to solve this kind of error, or since we already know it we can avoid it in future.

@cyphersnake
Copy link
Collaborator Author

unsafe is not some scary mechanism that scares developers if it is small and labeled safety

It seems to me you vastly overestimate the complexity this approach brings, it's a type from a standard library and a standard mechanism for initializing it. I first saw this mechanism in general in a regular web2 application, where developers are the least tempted to do complicated things

@cyphersnake
Copy link
Collaborator Author

Putting assert not zero everywhere and getting panic instead of compiler warning - that's the scary part 😅

@cyphersnake cyphersnake merged commit 4b70f36 into main Jan 4, 2024
1 check passed
@cyphersnake cyphersnake deleted the non-zero-constants branch January 4, 2024 18:35
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.

2 participants