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.

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

Implement split_pattern on slices #131340

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

eduardorittner
Copy link

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.

@rustbot
Copy link
Collaborator

rustbot commented Oct 6, 2024

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 6, 2024
Comment on lines 595 to 596
/// Allows splitting a slice by another slice
(unstable, split_pattern, "CURRENT_RUSTC_VERSION", Some(49036)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// 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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
split_pattern,

(see above)

Copy link
Member

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.

Copy link
Author

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!

@tgross35
Copy link
Contributor

tgross35 commented Oct 6, 2024

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_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.

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
@eduardorittner
Copy link
Author

Thank you so much for your help I issued an APC at rust-lang/libs-team#457 and will be going from there

@Amanieu Amanieu added S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants