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.

Sign up for GitHub

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

Jump to bottom

peek methods on iterators. #82

Open
blt-r opened this issue Mar 20, 2023 · 3 comments · May be fixed by #83
Open

peek methods on iterators. #82

blt-r opened this issue Mar 20, 2023 · 3 comments · May be fixed by #83

Comments

@blt-r
Copy link

blt-r commented Mar 20, 2023

In #18 (comment) there was a discussion about adding .peek_next() and .peek_prev() methods to iterators, but they were never added. And I'd like them to be added, because I have to do this:

let peek_prev = |chars: &mut ropey::iter::Chars| match chars.prev() {
    Some(_) => chars.next(),
    None => None,
};

And I would very much like to not do that. I don't know if compiler would optimize this away or if not, how bad is the overhead of doing this, but I assume this will be worse than proper implementation, especially if we hit that that worst case O(log N).

I can see in the code that implementing these methods would be not as trivial as just copy-pasting .next_impl() and .prev_impl() but removing the part that advances iterator (otherwise I would probably submit a PR).

I think these methods would be very useful, so it is probably worth implementing them
@cessen
Copy link
Owner

cessen commented Mar 25, 2023

Thanks for the reminder on this! I would still like to add these, yes.

I'm currently on vacation, so I'll respond in more detail after I get back.

@cessen
Copy link
Owner

cessen commented Apr 9, 2023

So, although I would like these to be added eventually, I don't consider it especially high priority.

However, I'd be happy to accept a PR with an initial implementation that more-or-less does what your code listing does: first advance the iterator to get the item, and then immediately undo that advance. It wouldn't be the most efficient implementation, but it would be easy to verify for correctness and would at least provide a more ergonomic API.

If you want to take a crack at a more efficient implementation, I think the (probably?) best approach would be to simply keep a cache of the previous and next items, which gets updated as the iterator advances. And then they're always just available immediately.

@blt-r
Copy link
Author

blt-r commented Apr 9, 2023

So this can be implemented similar to std::iter::Peekable, except it would also work with .prev(). The struct would have two more fields next_peeked and prev_peeked with Option<u8/char/&str>. In .peek_next() we will call .next_impl(), and put it into next_peeked and in .next(), if there is something to peeked, we will take that. Also, char and &str have a niche to fit the None variant, so Options with them won't be that big.

Yeah, I can try implementing this

@blt-r blt-r linked a pull request Apr 10, 2023 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Implement two-way iterators and peekables blt-r/ropey
2 participants
@cessen @blt-r