Add Peekable::next_if #72310
Merged
Add Peekable::next_if #72310
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @dtolnay (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
dtolnay
requested changes
May 22, 2020
There was a problem hiding this comment.
Seems plausible to me. Some remaining steps:
- Please open a tracking issue and change the stability attrs to
unstable. - Move both of these into the public impl block containing the peek method. Otherwise the docs come out very confusing with multiple impls and the most commonly relevant method way at the bottom. There should be only 1 impl block containing public methods.
- Move the Borrow bound to a
whereclause. It will be more readable in documentation. - Add tests in src/libcore/tests/iter.rs exercising the Borrow bound.
- I think your bound is actually backwards i.e. I would assume the point was to support next_if_eq("...") on a peekable of String.
3 tasks
Hmm, I really wanted Then I tried having a common type that they both borrow to, but now rustc can't infer the common type: |
dtolnay
requested changes
May 26, 2020
There was a problem hiding this comment.
- Please move the methods to the same impl block that contains
peek, which is further down. Otherwise there is still more than one block containing public methods, andpeekwhich is the most important one ends up too far down in the documentation. - I think we should remove the use of Borrow here. The method should take an argument of type
&RwhereItem: PartialEq<R>. Add Peekable::next_if #72310 (comment) sounds like fighting the language which is not usually good. That sort of thing would be better addressed by something like discarding ownership which will allow calling next_if_eq(0) without a more elaborate trait bound.
That's fair. If we remove the |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
You should try it. String implements PartialEq<str> so I would expect it to work. |
|
Using error[E0277]: can't compare `&str` with `str`
--> src/libcore/../libcore/tests/iter.rs:822:19
|
822 | assert_eq!(it.next_if_eq("trillian"), None);
| ^^^^^^^^^^ no implementation for `&str == str`
|
= help: the trait `std::cmp::PartialEq<str>` is not implemented for `&str`I tried using and adding explicit lifetimes doesn't help: error: lifetime may not live long enough
--> src/libcore/iter/adapters/mod.rs:1639:29
|
1630 | pub fn next_if_eq<'a, R>(
| -- lifetime `'a` defined here
...
1639 | self.next_if(|next| next == expected)
| ---- ^^^^ requires that `'1` must outlive `'a`
| |
| has type `&'1 <I as iter::traits::iterator::Iterator>::Item`
error: aborting due to previous errorI'm pushing the current implementation now but I'm not sure how to proceed, the errors are confusing me. |
|
Ah wait this is the same error as before, I need to use |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
dtolnay
requested changes
May 27, 2020
src/libcore/tests/iter.rs
Outdated
Comment on lines
836
to
845
| assert_eq!(it.next_if_eq("speed"), Some("speed".into())); | ||
| /* | ||
| assert_eq!(it.next_if_eq(""), None); | ||
| // try after peek() | ||
| assert_eq!(it.peek(), Some(&"of")); | ||
| assert_eq!(it.next_if_eq("of"), Some("of")); | ||
| assert_eq!(it.next_if_eq("zaphod"), None); | ||
| // make sure `next()` still behaves | ||
| assert_eq!(it.next(), Some("Gold")); | ||
| */ |
There was a problem hiding this comment.
This test needs to be finished.
There was a problem hiding this comment.
I think I'm just going to remove the commented-out section, it doesn't test anything that's not tested above.
Prior art: `rust_analyzer` uses [`Parser::eat`](https://github.com/rust-analyzer/rust-analyzer/blob/50f4ae798b7c54d417ee88455b87fd0477473150/crates/ra_parser/src/parser.rs#L94), which is `next_if` specialized to `|y| next_if(|x| x == y)`. Basically every other parser I've run into in Rust has an equivalent of Parser::eat; see for example - [cranelift](https://github.com/bytecodealliance/wasmtime/blob/94190d57244b26baf36629c88104b0ba516510cf/cranelift/reader/src/parser.rs#L498) - [rcc](https://github.com/jyn514/rcc/blob/a8159c3904a0c950fbba817bf9109023fad69033/src/parse/mod.rs#L231) - [crunch](https://github.com/Kixiron/crunch-lang/blob/8521874fab8a7d62bfa7dea8bd1da94b63e31be8/crates/crunch-parser/src/parser/mod.rs#L213-L241)
dtolnay
approved these changes
May 27, 2020
|
@bors r+ |
|
📌 Commit 822ad87 has been approved by |
bors
added
S-waiting-on-bors
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
May 27, 2020
Add Peekable::next_if Prior art: `rust_analyzer` uses [`Parser::eat`](https://github.com/rust-analyzer/rust-analyzer/blob/50f4ae798b7c54d417ee88455b87fd0477473150/crates/ra_parser/src/parser.rs#L94), which is `next_if` specialized to `|y| self.next_if(|x| x == y)`. Basically every other parser I've run into in Rust has an equivalent of `Parser::eat`; see for example - [cranelift](https://github.com/bytecodealliance/wasmtime/blob/94190d57244b26baf36629c88104b0ba516510cf/cranelift/reader/src/parser.rs#L498) - [rcc](https://github.com/jyn514/rcc/blob/a8159c3904a0c950fbba817bf9109023fad69033/src/parse/mod.rs#L231) - [crunch](https://github.com/Kixiron/crunch-lang/blob/8521874fab8a7d62bfa7dea8bd1da94b63e31be8/crates/crunch-parser/src/parser/mod.rs#L213-L241) Possible extensions: A specialization of `next_if` to using `Eq::eq`. The only difficulty here is the naming - maybe `next_if_eq`? Alternatives: - Instead of `func: impl FnOnce(&I::Item) -> bool`, use `func: impl FnOnce(I::Item) -> Option<I::Item>`. This has the advantage that `func` can move the value if necessary, but means that there is no guarantee `func` will return the same value it was given. - Instead of `fn next_if(...) -> Option<I::Item>`, use `fn next_if(...) -> bool`. This makes the common case of `iter.next_if(f).is_some()` easier, but makes the unusual case impossible. Bikeshedding on naming: - `next_if` could be renamed to `consume_if` (to match `eat`, but a little more formally) - `next_if_eq` could be renamed to `consume`. This is more concise but less self-explanatory if you haven't written a lot of parsers. - Both of the above, but with `consume` replaced by `eat`.
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
May 27, 2020
Add Peekable::next_if Prior art: `rust_analyzer` uses [`Parser::eat`](https://github.com/rust-analyzer/rust-analyzer/blob/50f4ae798b7c54d417ee88455b87fd0477473150/crates/ra_parser/src/parser.rs#L94), which is `next_if` specialized to `|y| self.next_if(|x| x == y)`. Basically every other parser I've run into in Rust has an equivalent of `Parser::eat`; see for example - [cranelift](https://github.com/bytecodealliance/wasmtime/blob/94190d57244b26baf36629c88104b0ba516510cf/cranelift/reader/src/parser.rs#L498) - [rcc](https://github.com/jyn514/rcc/blob/a8159c3904a0c950fbba817bf9109023fad69033/src/parse/mod.rs#L231) - [crunch](https://github.com/Kixiron/crunch-lang/blob/8521874fab8a7d62bfa7dea8bd1da94b63e31be8/crates/crunch-parser/src/parser/mod.rs#L213-L241) Possible extensions: A specialization of `next_if` to using `Eq::eq`. The only difficulty here is the naming - maybe `next_if_eq`? Alternatives: - Instead of `func: impl FnOnce(&I::Item) -> bool`, use `func: impl FnOnce(I::Item) -> Option<I::Item>`. This has the advantage that `func` can move the value if necessary, but means that there is no guarantee `func` will return the same value it was given. - Instead of `fn next_if(...) -> Option<I::Item>`, use `fn next_if(...) -> bool`. This makes the common case of `iter.next_if(f).is_some()` easier, but makes the unusual case impossible. Bikeshedding on naming: - `next_if` could be renamed to `consume_if` (to match `eat`, but a little more formally) - `next_if_eq` could be renamed to `consume`. This is more concise but less self-explanatory if you haven't written a lot of parsers. - Both of the above, but with `consume` replaced by `eat`.
dtolnay
added
the
T-libs-api
RalfJung
added a commit
to RalfJung/rust
that referenced
this pull request
May 29, 2020
Add Peekable::next_if Prior art: `rust_analyzer` uses [`Parser::eat`](https://github.com/rust-analyzer/rust-analyzer/blob/50f4ae798b7c54d417ee88455b87fd0477473150/crates/ra_parser/src/parser.rs#L94), which is `next_if` specialized to `|y| self.next_if(|x| x == y)`. Basically every other parser I've run into in Rust has an equivalent of `Parser::eat`; see for example - [cranelift](https://github.com/bytecodealliance/wasmtime/blob/94190d57244b26baf36629c88104b0ba516510cf/cranelift/reader/src/parser.rs#L498) - [rcc](https://github.com/jyn514/rcc/blob/a8159c3904a0c950fbba817bf9109023fad69033/src/parse/mod.rs#L231) - [crunch](https://github.com/Kixiron/crunch-lang/blob/8521874fab8a7d62bfa7dea8bd1da94b63e31be8/crates/crunch-parser/src/parser/mod.rs#L213-L241) Possible extensions: A specialization of `next_if` to using `Eq::eq`. The only difficulty here is the naming - maybe `next_if_eq`? Alternatives: - Instead of `func: impl FnOnce(&I::Item) -> bool`, use `func: impl FnOnce(I::Item) -> Option<I::Item>`. This has the advantage that `func` can move the value if necessary, but means that there is no guarantee `func` will return the same value it was given. - Instead of `fn next_if(...) -> Option<I::Item>`, use `fn next_if(...) -> bool`. This makes the common case of `iter.next_if(f).is_some()` easier, but makes the unusual case impossible. Bikeshedding on naming: - `next_if` could be renamed to `consume_if` (to match `eat`, but a little more formally) - `next_if_eq` could be renamed to `consume`. This is more concise but less self-explanatory if you haven't written a lot of parsers. - Both of the above, but with `consume` replaced by `eat`.
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
May 29, 2020
Add Peekable::next_if Prior art: `rust_analyzer` uses [`Parser::eat`](https://github.com/rust-analyzer/rust-analyzer/blob/50f4ae798b7c54d417ee88455b87fd0477473150/crates/ra_parser/src/parser.rs#L94), which is `next_if` specialized to `|y| self.next_if(|x| x == y)`. Basically every other parser I've run into in Rust has an equivalent of `Parser::eat`; see for example - [cranelift](https://github.com/bytecodealliance/wasmtime/blob/94190d57244b26baf36629c88104b0ba516510cf/cranelift/reader/src/parser.rs#L498) - [rcc](https://github.com/jyn514/rcc/blob/a8159c3904a0c950fbba817bf9109023fad69033/src/parse/mod.rs#L231) - [crunch](https://github.com/Kixiron/crunch-lang/blob/8521874fab8a7d62bfa7dea8bd1da94b63e31be8/crates/crunch-parser/src/parser/mod.rs#L213-L241) Possible extensions: A specialization of `next_if` to using `Eq::eq`. The only difficulty here is the naming - maybe `next_if_eq`? Alternatives: - Instead of `func: impl FnOnce(&I::Item) -> bool`, use `func: impl FnOnce(I::Item) -> Option<I::Item>`. This has the advantage that `func` can move the value if necessary, but means that there is no guarantee `func` will return the same value it was given. - Instead of `fn next_if(...) -> Option<I::Item>`, use `fn next_if(...) -> bool`. This makes the common case of `iter.next_if(f).is_some()` easier, but makes the unusual case impossible. Bikeshedding on naming: - `next_if` could be renamed to `consume_if` (to match `eat`, but a little more formally) - `next_if_eq` could be renamed to `consume`. This is more concise but less self-explanatory if you haven't written a lot of parsers. - Both of the above, but with `consume` replaced by `eat`.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 29, 2020
Rollup of 9 pull requests Successful merges: - rust-lang#72310 (Add Peekable::next_if) - rust-lang#72383 (Suggest using std::mem::drop function instead of explicit destructor call) - rust-lang#72398 (SocketAddr and friends now correctly pad its content) - rust-lang#72465 (Warn about unused captured variables) - rust-lang#72568 (Implement total_cmp for f32, f64) - rust-lang#72572 (Add some regression tests) - rust-lang#72591 (librustc_middle: Rename upvar_list to closure_captures) - rust-lang#72701 (Fix grammar in liballoc raw_vec) - rust-lang#72731 (Add missing empty line in E0619 explanation) Failed merges: r? @ghost
|
Sorry if this is not the best place to give feedback. I was actually looking for an iterator when I found the Becomes which is, IMO, even more useful when parsing. |
The tracking issue would be a better place: #72480
|
|
Sure, I can try that, that sounds fun. I haven't contributed here before, but I'll get the inspiration from this PR! Thanks. |
Closed
pickfire
reviewed
Sep 2, 2020
| /// assert_eq!(iter.next(), Some(1)); | ||
| /// ``` | ||
| #[unstable(feature = "peekable_next_if", issue = "72480")] | ||
| pub fn next_if_eq<R>(&mut self, expected: &R) -> Option<I::Item> |
There was a problem hiding this comment.
Is there any reason why this is R? Why not T?
There was a problem hiding this comment.
I think R for reference? I don't remember any more, I'll switch it to T.
pickfire
reviewed
Sep 2, 2020
| /// Otherwise, return `None`. | ||
| /// | ||
| /// # Examples | ||
| /// Consume a number if it's equal to 0. |
There was a problem hiding this comment.
Isn't this description duplicated from the comments in the example?
There was a problem hiding this comment.
This is the same description, but using a different implementation.
pickfire
reviewed
Sep 2, 2020
| @@ -1574,6 +1574,69 @@ impl<I: Iterator> Peekable<I> { | |||
| let iter = &mut self.iter; | |||
| self.peeked.get_or_insert_with(|| iter.next()).as_ref() | |||
| } | |||
|
|
|||
| /// Consume the next value of this iterator if a condition is true. | |||
There was a problem hiding this comment.
I believe Consume and return or Take may be a better choice?
matklad
added a commit
to matklad/rust
that referenced
this pull request
Sep 4, 2020
Address review comments on `Peekable::next_if` r? @pickfire See rust-lang#72310 (review) for context.
matklad
added a commit
to matklad/rust
that referenced
this pull request
Sep 4, 2020
Address review comments on `Peekable::next_if` r? @pickfire See rust-lang#72310 (review) for context.
matklad
added a commit
to matklad/rust
that referenced
this pull request
Sep 4, 2020
Address review comments on `Peekable::next_if` r? @pickfire See rust-lang#72310 (review) for context.
matklad
added a commit
to matklad/rust
that referenced
this pull request
Sep 4, 2020
Address review comments on `Peekable::next_if` r? @pickfire See rust-lang#72310 (review) for context.
matklad
added a commit
to matklad/rust
that referenced
this pull request
Sep 4, 2020
Address review comments on `Peekable::next_if` r? @pickfire See rust-lang#72310 (review) for context.
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Sep 6, 2020
Address review comments on `Peekable::next_if` r? @pickfire See rust-lang#72310 (review) for context.
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Sep 6, 2020
Address review comments on `Peekable::next_if` r? @pickfire See rust-lang#72310 (review) for context.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior art:
rust_analyzerusesParser::eat, which isnext_ifspecialized to|y| self.next_if(|x| x == y).Basically every other parser I've run into in Rust has an equivalent of
Parser::eat; see for examplePossible extensions: A specialization of
next_ifto usingEq::eq. The only difficulty here is the naming - maybenext_if_eq?Alternatives:
func: impl FnOnce(&I::Item) -> bool, usefunc: impl FnOnce(I::Item) -> Option<I::Item>. This has the advantage thatfunccan move the value if necessary, but means that there is no guaranteefuncwill return the same value it was given.fn next_if(...) -> Option<I::Item>, usefn next_if(...) -> bool. This makes the common case ofiter.next_if(f).is_some()easier, but makes the unusual case impossible.Bikeshedding on naming:
next_ifcould be renamed toconsume_if(to matcheat, but a little more formally)next_if_eqcould be renamed toconsume. This is more concise but less self-explanatory if you haven't written a lot of parsers.consumereplaced byeat.