Implement split_pattern on slices by eduardorittner · Pull Request #131340 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or 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
[ Show hidden characters]({{ revealButtonHref }})
From #49036, adds a split_pattern method on slices which takes a second slice and splits using that one.
I was unsure about where to add additional tests, specifically to test reverse iteration using the next_back method, my guess would be in library/core/tests/slice.rs, however when looking at the tests I saw no test for unstable features, so I was unsure if I should add them somewhere else.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) some time within the next two weeks.
Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:
@rustbot author: the review is finished, PR author should check the comments and take action accordingly@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
Relevant to the library team, which will review and decide on the PR/issue.
labels
Comment on lines 595 to 596
| /// Allows splitting a slice by another slice |
|---|
| (unstable, split_pattern, "CURRENT_RUSTC_VERSION", Some(49036)), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// Allows splitting a slice by another slice |
|---|
| (unstable, split_pattern, "CURRENT_RUSTC_VERSION", Some(49036)), |
Library features shouldn't (and don't need to) be declared inside the compiler.
| @@ -1864,6 +1864,7 @@ symbols! { |
|---|
| soft, |
| specialization, |
| speed, |
| split_pattern, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This UI test should be removed. It's only required to exist right now by tidy because you declared a library feature as a compiler feature, too. The doctest should be sufficient.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! I was following the rustc-dev-guide and thought that all unstable features should be declared in the compiler, thanks for clarifying!
This should get proposed via an ACP, which is one of the issue templates at https://github.com/rust-lang/libs-team/issues. That gives the libs-api team a chance to figure out if the new API makes sense before you put too much effort into the implementation.
I was unsure about where to add additional tests, specifically to test reverse iteration using the
next_backmethod, my guess would be in library/core/tests/slice.rs, however when looking at the tests I saw no test for unstable features, so I was unsure if I should add them somewhere else.
As fmease mentioned, library features don't really need to touch anything outside of library/, no compiler changes are needed. library/core/tests/iter would be a reasonable place to test this.
Since it's a library feature it shouldn't be declared inside the compiler
Labels
Status: PR has an ACP and is waiting for the ACP to complete.
Relevant to the compiler team, which will review and decide on the PR/issue.
Relevant to the library team, which will review and decide on the PR/issue.