Add PeekableIterator trait as an experiment #95195
Conversation
|
r? @m-ou-se (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
clarfonthey
force-pushed
the
peekable_iterator
branch
from
cbedf52 to
0484314
Compare
This comment has been minimized.
This comment has been minimized.
clarfonthey
force-pushed
the
peekable_iterator
branch
2 times, most recently
from
5dc62c3 to
d600886
Compare
clarfonthey
force-pushed
the
peekable_iterator
branch
from
d600886 to
5ab3c2f
Compare
| /// assert_eq!(Some(0), five.peek()); | ||
| /// ``` | ||
| #[unstable(feature = "peekable_iterator", issue = "none")] | ||
| fn peek(&self) -> Option<Self::Item>; |
There was a problem hiding this comment.
Can you say more about why you picked Option<Item> instead of Option<&Item> here? Especially since that's what peek does https://doc.rust-lang.org/std/iter/struct.Peekable.html#method.peek
And it'd make sense to me for things like Repeat to return a reference to the state, for example.
There was a problem hiding this comment.
The big reason is that a majority of iterators are completely impossible to do with that type signature. By-reference and by-mutable-reference iterators over collections would completely break, and that's loads of use cases right there.
At first I did that, then I realised how bad of an idea it would be to do that. Essentially, the use cases that would prefer just storing the next value and then referencing it are covered by peekable, and this covers the ones where that seems suboptimal.
There was a problem hiding this comment.
But that leads to some implementations using clone(), and has_next() just calls peek() which then clones. That calling has_next() might allocate is going to be surprising and certainly more costly than ExactSizeIterator's is_empty().
Edit: Ah, you mention this already in the PR comment.
But The marker trait approach by @scottmcm in #35428 (comment) seems like a more general suggestion. |
|
☔ The latest upstream changes (presumably #94901) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I'm going to close this for now since I feel like this implementation could use way more work. But if anyone wants to have a go, feel free to reuse what I wrote in the meantime. |
This is a potential solution to the messy debate surrounding
ExactSizeIterator::is_emptybeing moved to a separate trait. (tracked in #35428)I tried to not put too much effort into this since it'd be a load of work implementing it for every possible iterator in libstd, but at least for now, this is a proof-of-concept that is implemented for most of the adapters for which it made sense.
One major unanswered question already: should cloning or running functions be considered low-cost enough for a peek, since
has_nextcould be specialized to not do them? Technically, they support peeking, but it seems wrong to implement it regardless.Another open question is how
DoubleEndedPeekableIteratorwould exist, if it would at all.