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

Add PeekableIterator trait as an experiment #95195

Closed
wants to merge 1 commit into from

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Mar 22, 2022

This is a potential solution to the messy debate surrounding ExactSizeIterator::is_empty being 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_next could be specialized to not do them? Technically, they support peeking, but it seems wrong to implement it regardless.

Another open question is how DoubleEndedPeekableIterator would exist, if it would at all.

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 22, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@clarfonthey clarfonthey force-pushed the peekable_iterator branch 2 times, most recently from 5dc62c3 to d600886 Compare March 22, 2022 03:41
/// assert_eq!(Some(0), five.peek());
/// ```
#[unstable(feature = "peekable_iterator", issue = "none")]
fn peek(&self) -> Option<Self::Item>;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@clarfonthey clarfonthey Mar 22, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member

@the8472 the8472 Mar 22, 2022

Choose a reason for hiding this comment

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

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.

@the8472
Copy link
Member

the8472 commented Mar 22, 2022

This is a potential solution to the messy debate surrounding ExactSizeIterator::is_empty being moved to a separate trait.

But ExactSizeIterator can be implemented on more adapters, e.g. those that have side-effects such as Map.

The marker trait approach by @scottmcm in #35428 (comment) seems like a more general suggestion.

@bors
Copy link
Contributor

bors commented Mar 23, 2022

☔ The latest upstream changes (presumably #94901) made this pull request unmergeable. Please resolve the merge conflicts.

@clarfonthey
Copy link
Contributor Author

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.

@clarfonthey clarfonthey deleted the peekable_iterator branch March 27, 2022 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants