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

Resolve arith overflow on with_capacity #628

Merged
merged 3 commits into from Nov 10, 2023

Conversation

HeeillWang
Copy link
Contributor

resolve #626 #627

Copy link
Contributor

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

this looks good to me overall, thanks for the fix!

i had some minor suggestions for improving the panic messages so that the requested capacity is displayed to the user. what do you think?

src/header/map.rs Outdated Show resolved Hide resolved
src/header/map.rs Outdated Show resolved Hide resolved
@WhyNotHugo
Copy link

Function docstring should clarify this function panics:

/// # Panics
/// 
/// Requested capacity too large: would overflow `usize`.

@HeeillWang
Copy link
Contributor Author

Function docstring should clarify this function panics:

/// # Panics
/// 
/// Requested capacity too large: would overflow `usize`.

reflected!

Comment on lines +3231 to +3237
match n.checked_add(n / 3) {
Some(n) => n,
None => panic!(
"requested capacity {} too large: overflow while converting to raw capacity",
n
),
}
Copy link

Choose a reason for hiding this comment

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

TBH, this could just be:

n.checked_add(n / 3).expect("requested capacity must fit usize without overflow")

The only downside if that the requested capacity is not shown in the error, so maybe you'll want to hear what the maintainers think before applying any changes.

(the same applies to the above match that basically hand-rolls an expect).

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.

Arithmetic overflow found on to_raw_capacity()
4 participants