-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Implement some trivial size_hints for various iterators #49201
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
Conversation
This also implements ExactSizeIterator where applicable. Addresses most of the Iterator traits mentioned in rust-lang#23708.
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
LGTM, @rust-lang/libs anything I might be overlooking? |
src/libcore/char.rs
Outdated
|
||
#[inline] | ||
fn size_hint(&self) -> (usize, Option<usize>) { | ||
self.0.size_hint() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This look incorrect. This iterator can yield fewer char
s than it has input u8
s. See std::str::Chars::size_hint
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right you are. I saw self.0.next().map(...)
in the next()
implementation and assumed that meant a 1:1 correspondence.
src/libcore/iter/traits.rs
Outdated
@@ -901,6 +901,10 @@ impl<I, T, E> Iterator for ResultShunt<I, E> | |||
None => None, | |||
} | |||
} | |||
|
|||
fn size_hint(&self) -> (usize, Option<usize>) { | |||
self.iter.size_hint() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks incorrect since this iterator can yield fewer items than self.iter
. Also this particular iterator is private and as far as can tell its size_hint
is never used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this should instead be:
let (_, upper) = self.iter.size_hint();
(0, upper)
And I guess it's never used? I didn't really look; figuring out if it's used is harder than implementing it just-in-case, and the code around it could change.
I can remove it if you like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it’s not used but this implementation is small enough that it’s probably fine to add it just in case it becomes used later.
src/libcore/option.rs
Outdated
|
||
#[inline] | ||
fn size_hint(&self) -> (usize, Option<usize>) { | ||
self.iter.size_hint() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks incorrect since this iterator can yield fewer items than self.iter
. See std::iter::Filter::size_hint
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, forgot to commit this file. I'll commit and push it tomorrow.
r? @SimonSapin |
It looks like (Please comment after pushing, I sometimes get email notifications for new commits but not always. I haven’t figured out in what cases.) |
Pushed my code for |
Thanks! @bors: r+ |
📌 Commit 5057e3c has been approved by |
Implement some trivial size_hints for various iterators This also implements ExactSizeIterator where applicable. Addresses most of the Iterator traits mentioned in #23708. I intend to do more, but I don't want to make the PR too large.
☀️ Test successful - status-appveyor, status-travis |
This also implements ExactSizeIterator where applicable.
Addresses most of the Iterator traits mentioned in #23708.
I intend to do more, but I don't want to make the PR too large.