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

sanitize_uint improvement #148

Merged
merged 2 commits into from
Mar 15, 2023
Merged

sanitize_uint improvement #148

merged 2 commits into from
Mar 15, 2023

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Mar 9, 2023

this patch separates overflow errors from invalid integer representation errors in the sanitize_uint() function. The overflow behaviors are almost always subject to customizations, depending on who's calling it. The invalid integer representations on the other hand, should always just propagate up.

This simplifies the call sites and removes one intermediate layer of functions that currently customize the overflow behavior. Now it's done at each call site.

@arvidn arvidn marked this pull request as ready for review March 9, 2023 09:34
Copy link
Contributor

@AmineKhaldi AmineKhaldi left a comment

Choose a reason for hiding this comment

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

Looks good! I left some comments.

fuzz/fuzz_targets/sanitize-uint.rs Show resolved Hide resolved
src/gen/conditions.rs Show resolved Hide resolved
src/gen/conditions.rs Show resolved Hide resolved
src/gen/conditions.rs Outdated Show resolved Hide resolved
@arvidn arvidn force-pushed the sanitize-int branch 2 times, most recently from b79fa61 to ef1f8a8 Compare March 10, 2023 14:00
@arvidn arvidn requested a review from AmineKhaldi March 13, 2023 15:33
@aqk
Copy link

aqk commented Mar 15, 2023

Result<SanitizedUint, ValidationErr> is so much better than <&[u8], ValidationErr>

@arvidn arvidn merged commit 5ff7c31 into main Mar 15, 2023
@arvidn arvidn deleted the sanitize-int branch March 15, 2023 07:37
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