Untangle InterleaveShortest::size_hint and fix #1068#1112
Open
phimuemue wants to merge 13 commits into
Open
Conversation
Simplifies subsequent diffs. The function was not used anywhere else, so I cut-pasted it.
…int_min Simplifies subsequent diffs. The function is in other places, so I copy-pasted it.
Before this commit, we repeated the comparison between `curr_lower` and `next_lower`, because we did it explicitly, and implicitly through `cmp::min`. Now, because we rely on one comparison. This simplifies the logic: If `curr_lower > next_lower`, then the "next" iterator is the limitting factor, and there's one additional element at the end coming from the "current" iterator. Otherwise, the "current" iterator is the limitting factor, and for each element of the "current" iterator, there's one from the "next" iterator, so we do not have to add 1.
This avoids constructing an `Option`, just to later inspect it and map it via `and_then`. It's a bit more code, but I'd argue it's clearer.
We now do the `checked_add(1)` inline instead of computing `extra_elem` and then looking back to see if we shall add 1. Now, the computation of `lower` and `upper` is (at least somewhat) in line: Check if curr or next is the limitting factor and behave accordingly.
If "next" is the limitting factor, there may be one element from "curr" at the end. But if this element leads to more than `usize::MAX` elements, a lower bound of `usize::MAX` is still good enough. Fixes rust-itertools#1068
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1112 +/- ##
==========================================
- Coverage 94.38% 93.98% -0.40%
==========================================
Files 48 52 +4
Lines 6665 6671 +6
==========================================
- Hits 6291 6270 -21
- Misses 374 401 +27 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While reviewing #1109, I realized that
InterleaveShortest::size_hintis hard too understand, mainly because we compare lower/upper bounds separately and with the magicsize_hint::minmethods.I kept my commits so that you can follow step-by-step how I untangled it. At the end, we do - both for the
size_hint's lower and upper bound - this: Compare "current" vs "next" iterator's estimated length, and act accordingly.interleave_shortest's size_hint panics with large iterators. #1068Thanks @patrickwehbe, @c-tonneslan and @ronnodas for your ideas on this.