cessen / ropey

A utf8 text rope for manipulating and editing large texts.
MIT License
1.04k stars 46 forks source link

peek methods on iterators. #82

Open blt-r opened 1 year ago

blt-r commented 1 year ago

In https://github.com/cessen/ropey/issues/18#issuecomment-522326596 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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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