Github Stabilize `peekable_next_if` by Stupremee · Pull Request #80011 · rust-la...

Member

rust-highfive commented on Dec 13, 2020

r? @withoutboats

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

Stupremee commented on Dec 13, 2020

r? @jyn514

jyn514 commented on Dec 13, 2020

I'm not on T-libs, I shouldn't be the reviewer. r? @dtolnay maybe since they reviewed the initial PR, but it will need FCP either way.

dtolnay commented on Dec 14, 2020

@rust-lang/libs:
@rfcbot fcp merge

These two methods on std::iter::Peekable:

  impl<I: Iterator> Peekable<I> {
+     pub fn next_if(&mut self, func: impl FnOnce(&I::Item) -> bool) -> Option<I::Item>;

+     pub fn next_if_eq<T>(&mut self, expected: &T) -> Option<I::Item>
+     where
+         T: ?Sized,
+         I::Item: PartialEq<T>;
  }

let alnum = peekable.next_if(char::is_ascii_alphanumeric);

// equivalent to:
let alnum = match peekable.peek() {
    Some(ch) if ch.is_ascii_alphanumeric() => Some(peekable.next().unwrap()),
    _ => None,
};

https://doc.rust-lang.org/nightly/std/iter/struct.Peekable.html#method.next_if
https://doc.rust-lang.org/nightly/std/iter/struct.Peekable.html#method.next_if_eq

rfcbot commented on Dec 14, 2020 •

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

• mut resolved by #80011 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

m-ou-se commented on Dec 14, 2020 •

@rfcbot concern mut

Now that Peekable::peek_mut also exists (#77491), I'm wondering if next_if should provide a &mut Item rather than a &Item. (To not make the same mistake as with Vec::retain, which in hindsight could've/should've given a &mut but only gives a &.) What do you all think?

jyn514 commented on Dec 14, 2020

Why would you need to mutate the item?

KodrAus commented 10 days ago

I'm wondering if next_if should provide a &mut Item rather than a &Item

Oh that's an interesting philosophical question... Should we always try offer up the most exclusive reference we can?

Why would you need to mutate the item?

I wonder if we asked the same question about Vec::retain. The problem there is that in order to both retain and mutate you have to iterate the collection twice, isn't it?

With next_if it looks like there isn't much difference between next_if(filter).map(mutate) and a next_if_mut(filter_and_mutate_in_place). Since we stash the value in peeked, the semantics of next_if(|v| { mutate(v); false }) also seem a bit strange.

But I think is a shared reference sufficient when we could use an exclusive one? is a good question we should keep asking!

KodrAus commented 10 days ago

A next_if_mut method also doesn’t seem too offensive to me if we did want one. At least when reviewing code the _mut suffix would suggest there’s something more going on than just filtering and yielding values.

KodrAus commented 9 days ago

@m-ou-se Do you feel strongly that next_if should accept a &mut item, or are you satisfied with some combination of .peek_mut(mutate).next_if(condition_after_mutate) or .peek().next_if(condition).map(mutate), with a possible next_if_mut alternative introduced later if it proves useful?

m-ou-se commented 9 days ago

That sounds good to me.

@rfcbot resolve mut

rfcbot commented 9 days ago

This is now entering its final comment period, as per the review above.