Implement ExactSizeIterator for remaining core Iterators where applicable #21453
Conversation
…able. Specifically: * Peekable * ByRef * Skip * Take * Fuse
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see CONTRIBUTING.md for more information. |
|
I'd be happy to add unit tests if you want these changes. I can also break this into multiple pull requests/commits but that seemed like overkill. |
| #[stable] | ||
| impl<I> ExactSizeIterator for Skip<I> where I: ExactSizeIterator { | ||
| #[inline] | ||
| fn len(&self) -> uint { self.iter.len().saturating_sub(self.n) } |
There was a problem hiding this comment.
usize (and above and below)
|
It would be nice to test some of these, I think. It's partially non-trivial code, especially if we're going to start using these to do "trust" iterators in unsafe code. |
|
(I belive the tests would go in libcoretest) |
|
Quick question, should I be manually implementing Aside: Personally, I don't think traits should impose requirements on methods of other traits (too fragile) and |
| // value before we peeked. | ||
| self.iter.len() + if self.peeked.is_some() { 1 } else { 0 } | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think we can quite add this implementation just yet as I believe it's possible to return (usize::MAX, None) from this iterator (which isn't an exact size).
There was a problem hiding this comment.
We can because, given an underlying iterator that implements ExactSizeIterator, either:
- We haven't peeked:
self.peekedisNoneand we forward the result fromself.iter.size_hint()which must meet theExactSizeIteratorspec and must be at most(uint::MAX, Some(uint::MAX-1)). - We have peeked: To have peeked, we must have called
next()onself.itersoself.iter'ssize_hint()must be at most(lo, Some(hi)wherelo == hi <= uint::MAX-1because it must have met theExactSizeIteratorspec both before and after we peeked. Therefore adding 1 cannot overflow.
There was a problem hiding this comment.
which must meet the ExactSizeIterator spec and must be at most (uint::MAX, Some(uint::MAX-1)).
The size_hint documentation does not mention MAX - 1 and I was not under the impression that's what it was used for. I'm curious, but where'd you get the MAX - 1 from?
There was a problem hiding this comment.
Sorry, that was supposed to be (uint::MAX, Some(uint::MAX)) (copy and paste error).
My basic point is that that if the number of items in an ExactSizeIterator once fit in a uint (required by the spec), it must always fit in a uint regardless of where those items are (moving one from self.iter to self.peeked doesn't change the total number of items).
I'd leave it as a default method for now personally. |
|
Yeah I don't think there's a non-trivial |
ByRef is not tested included because it is a trivial pass through.
|
I disagree that the adapters should use the default impl. If I write an implementation of |
| @@ -1791,6 +1794,9 @@ impl<T, I> Iterator for Peekable<T, I> where I: Iterator<Item=T> { | |||
| } | |||
|
|
|||
| #[stable] | |||
| impl<T, I> ExactSizeIterator for Peekable<T, I> where I: ExactSizeIterator<Item = T> {} | |||
There was a problem hiding this comment.
(copying over from previous comment to continue discussion)
Sorry, that was supposed to be (uint::MAX, Some(uint::MAX)) (copy and paste error).
My basic point is that that if the number of items in an ExactSizeIterator once fit in a uint (required by the spec), it must always fit in a uint regardless of where those items are (moving one from self.iter to self.peeked doesn't change the total number of items).
Ah right, good point! So if the underlying iterator returns (MAX, Some(MAX)) then the only time we'll add 1 is if we've already peeked in which case the underlying iterator will return (MAX-1, Some(MAX-1)).
I agree but the spec would need to be changed. Otherwise, it would be too easy to end up with conflicting implementations of |
|
I think for now we're certainly stepping forward with these extra implementations. We may want to sort out whether delegation to |
Specifically: * Peekable * ByRef * Skip * Take * Fuse Fixes #20547
Specifically:
Fixes #20547