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

Fix subtraction overflow in DuplicatesBy size_hint #552

Merged
merged 4 commits into from Aug 16, 2021
Merged

Fix subtraction overflow in DuplicatesBy size_hint #552

merged 4 commits into from Aug 16, 2021

Conversation

arpankapoor
Copy link
Contributor

Sample that panics with attempt to subtract with overflow in a debug build:

use itertools::Itertools;

fn main() {
    println!("{:?}", vec!["a", "b", "c", "c"].iter().duplicates().collect_vec());
}

@phimuemue
Copy link
Member

Thanks for this. Yes, cmp::max(a-b, 0) seems strange on usize. I do not have much time right now, so just two questions:

  • Do you know if there are other occurences of this pattern in the code? (A "no, I do not know" is fine, we should just look ourselves, then.)
  • Should we add tests for this case?

@arpankapoor
Copy link
Contributor Author

  • I haven't seen this pattern repeated elsewhere on a cursory glance.
  • Added a test similar to the above example. Let me know if that seems sufficient.

I've also changed the checked_sub call to saturating_sub as per clippy's suggestion.

Comment on lines 91 to 93
let max_pending = std::cmp::min(self.meta.pending, hi);
let max_new = std::cmp::max(hi - self.meta.pending, 0) / 2;
let max_new = hi.saturating_sub(self.meta.pending) / 2;
max_pending + max_new
Copy link
Member

Choose a reason for hiding this comment

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

Hi there, one thing that bothers me is that cmp::min and saturating_sub actually answer the same question twice. (Namely: is hi <= self.meta.pending?)

I think it would be clearer to do this:

if hi <= self.meta.pending {
    // fewer or equally many iter-remaining elements than pending elements
    // => at most, each iter-remaining element is matched
    hi
} else {
    // fewer pending elements than iter-remaining elements
    // => at most:
    //    * each pending element is matched
    //    * the other iter-remaining elements come in pairs
    self.meta.pending + (hi-self.meta.pending)/2
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated as per your suggestion

@phimuemue
Copy link
Member

Thank you.

bors r+

@phimuemue
Copy link
Member

Merged as this improves the current size_hint. Maybe we should use our size_hint::add et al instead of plain + and others.

@bors
Copy link
Contributor

bors bot commented Aug 16, 2021

Build succeeded:

@bors bors bot merged commit 2c82c5e into rust-itertools:master Aug 16, 2021
@jswrenn jswrenn added this to the next milestone Sep 13, 2021
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.

None yet

3 participants