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

Implement a Peekable trait for Chars, CharIndices, slice::Iter etc. #33881

Closed
sandeep-datta opened this issue May 26, 2016 · 10 comments
Closed
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@sandeep-datta
Copy link
Contributor

sandeep-datta commented May 26, 2016

If you want to peek an iterator right now then you have do something on the following lines...

let chars = "abc".chars();
let mut peekable = chars.peekable();
println!("{:?}", peekable.peek());

This is unnecessarily complicated and unergonomic in the case of iterators like Chars, CharIndices, slice::Iter etc. IMO it would be better to define a Peekable trait as follows...

trait Peekable : Iterator {
    fn peek(&mut self) -> Option<&Self::Item>;
}

and using this iterator as a bound on the aforementioned concrete iterators (perhaps replacing the Iterator bound). Doing so will not only improve the ergonomics for the library user but also help in providing a less roundabout implementation for the peek() method. Iterators which cannot implement Peekable directly should continue to use the Peekable adapter.

This will change the first snippet as follows...

let chars = "abc".chars();
println!("{:?}", chars.peek());
@Stebalien
Copy link
Contributor

Nit: It would probably be called Peek, not Peekable.

Note: Peekable could specialize on Peek (lower overhead).

@sandeep-datta
Copy link
Contributor Author

@Stebalien thanks for your comment. I am okay with Peek as the trait name. I am afraid I don't know enough rust yet to understand what you mean by specialize here? Does rust have template (partial) specialization like C++?

@Stebalien
Copy link
Contributor

@sandeep-datta Currently, Peekable introduces some runtime overhead (it has to check if something has been peeked and has to store peeked elements). Adding a Peek trait would allow the Peekable struct forward the peek call to the inner iterator if it implements peek instead of implementing the peek logic itself.

@sandeep-datta
Copy link
Contributor Author

@Stebalien going through the list of iterators it appears that any trait which derives from the Iterator trait ends with an Iterator in its name. Examples: DoubleEndedIterator, ExactSizeIterator. I propose renaming Peek to PeekableIterator. What do you say?

@Stebalien
Copy link
Contributor

Sigh... You're right. This is an iterator thing from the days before the convention. For everything else, traits are named after their primary function (Write, Read, From, Into, AsRef, Borrow, ...).

@sandeep-datta
Copy link
Contributor Author

Okay so I tried to implement PeekableIterator for Chars but it seems the best I could do was ...

impl<'a> PeekableIterator for Chars<'a> {
    #[inline]
    fn peek(&self) -> Option<Self::Item> {
        self.clone().next()
    } 
}

since I do not want to store the peeked valued in an extra field inside Chars. Doing so would replicate what the Peekable struct already does and also violate the pay as you go principle for people who are not interested in peeking an iterator.

I think Peekable may the best we can do for now, at least for iterators like Chars, CharIndices. These iterators do not simply return a value from an underlying backing store they need to compute the next() value hence you may need to clone the inferior iterator. I think it is better that the users do this explicitly.

However PeekableIterator may still be useful for things like slices.

@sandeep-datta
Copy link
Contributor Author

Okay revisiting this topic I feel PeakableIterator on Chars still makes sense because Peekable takes ownership of the inferior iterator and hides it. There is no way of accessing its as_str() method.

@steveklabnik steveklabnik added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed A-libs labels Mar 24, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Jul 25, 2017
@dtolnay
Copy link
Member

dtolnay commented Nov 18, 2017

I think the idea of a peek trait is super promising. Thanks for filing this! Other iterators have started to handle peek in an ad-hoc way, for example linked_list::IterMut::peek_next. It would be great to generalize these and make them usable in a generic context using a trait.

There are a lot of difficult design tradeoffs though and I am not confident that we will be able to arrive at a consensus on a design here, so I would prefer to see this addressed in an RFC.

@dtolnay dtolnay closed this as completed Nov 18, 2017
@aslilac
Copy link

aslilac commented Oct 21, 2022

has there been any further discussion on this? this'd be super handy

@bluebear94
Copy link
Contributor

This would also allow using the peek methods on Chars while still being able to call as_str() on the iterator at the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants