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

Tracking Issue for slice_split_once #112811

Open
2 of 4 tasks
Benjamin-L opened this issue Jun 19, 2023 · 10 comments
Open
2 of 4 tasks

Tracking Issue for slice_split_once #112811

Benjamin-L opened this issue Jun 19, 2023 · 10 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Benjamin-L
Copy link
Contributor

Benjamin-L commented Jun 19, 2023

Feature gate: #![feature(slice_split_once)]

This is a tracking issue for the slice::split_once and slice::rsplit_once methods, which split a slice into two chunks on the first occurrence of a single-element delimiter. These mirror the existing stable str::split_once and str::rsplit_once. I expect these to be mostly used for ad-hoc parsing data that is not UTF-8.

Public API

// core::slice

impl [T] {
    pub fn split_once<F>(&self, pred: F) -> Option<(&[T], &[T])>
    where
        F: FnMut(&T) -> bool;
    pub fn rsplit_once<F>(&self, pred: F) -> Option<(&[T], &[T])>
    where
        F: FnMut(&T) -> bool;
}

Steps / History

Unresolved Questions

  • Should we include split_once_mut and rsplit_once_mut?

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@Benjamin-L Benjamin-L added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 19, 2023
Benjamin-L added a commit to Benjamin-L/rust that referenced this issue Jun 20, 2023
Feature gate is slice_split_once and tracking issue is rust-lang#112811.
@matklad
Copy link
Member

matklad commented Jul 17, 2023

Hm, I would have expect the following signature here:

pub fn split_once<F>(&self, delim: &[T]) -> Option<(&[T], &[T])>

With the predicate, we allow only single-element delimiters, which I think lowers the added utility here. With slice-delimiter, this function also encapsulates index + delim.len() computation.

With str and Pattern API we of course get all three of T, Fn(&T) -> bool, &[T] delimiters, but my personal experience is that the third version is by far the most useful one (and the first version is a special-case of the third anyway).

@Benjamin-L
Copy link
Contributor Author

Yeah, I generally think &[T] is more useful too, however all of the existing split methods in slices use a single-element predicate. Since these new methods is supposed to be analogous with the existing slice::{split,rsplit} methods, I think using &[T] here would be particularly confusing.

It's probably too late to introduce a Pattern-like API for slices in a backwards-compatible way, or switch the existing methods to work with &[T]. Either way, that should probably be it's own change.

bors added a commit to rust-lang-ci/rust that referenced this issue Oct 9, 2023
…uviper

Implement `slice::split_once` and `slice::rsplit_once`

Feature gate is `slice_split_once` and tracking issue is rust-lang#112811. These are equivalents to the existing `str::split_once` and `str::rsplit_once` methods.
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 10, 2023
…uviper

Implement `slice::split_once` and `slice::rsplit_once`

Feature gate is `slice_split_once` and tracking issue is rust-lang#112811. These are equivalents to the existing `str::split_once` and `str::rsplit_once` methods.
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 11, 2023
…uviper

Implement `slice::split_once` and `slice::rsplit_once`

Feature gate is `slice_split_once` and tracking issue is rust-lang#112811. These are equivalents to the existing `str::split_once` and `str::rsplit_once` methods.
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 11, 2023
…uviper

Implement `slice::split_once` and `slice::rsplit_once`

Feature gate is `slice_split_once` and tracking issue is rust-lang#112811. These are equivalents to the existing `str::split_once` and `str::rsplit_once` methods.
@huonw
Copy link
Member

huonw commented Oct 23, 2023

For slices with arbitrary T, the split element isn't necessarily obvious or simple. Is it worth returning Option<(&[T], &T, &[T])> to have easier direct access to that element? (If/when a &mut version is added, this will be required, because reindexing to get the split element won't work.)

@Benjamin-L
Copy link
Contributor Author

Hmm, yeah I like that in the abstract. I think the main argument against it is that it's inconsistent with the existing split_* methods on both str and slice. I've definitely wanted something similar in the past when using str::split_once with a function pattern, so I don't think that the reasons for wanting access to the split delimiter are specific to arbitrary T.

@cls
Copy link
Contributor

cls commented Jan 6, 2024

The doc comments for these methods look to have a typo currently, which I just tripped over in the docs: "If any matching elements are resent in the slice [...]". After a moment of confusion ("resent? re-sent? recent?"), I realised that they were probably meant to read present. I'd happily patch it but I'm not familiar with the process, so I thought I'd just mention it here instead.

@jhpratt
Copy link
Member

jhpratt commented Jan 6, 2024

"present" is presumably correct. As for process, just open a PR. Someone will be assigned to review it — don't worry if it takes a while.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 6, 2024
…Denton

Fix typo in docs for slice::split_once, slice::rsplit_once

This fixes a typo in the doc comments for these methods, which I tripped over while reading the docs: "If any matching elements are **resent** in the slice [...]", which is presumably meant to read **present**.

I mentioned this in rust-lang#112811, the tracking issue for `slice_split_once`, and was encouraged to open a PR.
compiler-errors added a commit to compiler-errors/rust that referenced this issue Jan 7, 2024
…Denton

Fix typo in docs for slice::split_once, slice::rsplit_once

This fixes a typo in the doc comments for these methods, which I tripped over while reading the docs: "If any matching elements are **resent** in the slice [...]", which is presumably meant to read **present**.

I mentioned this in rust-lang#112811, the tracking issue for `slice_split_once`, and was encouraged to open a PR.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 7, 2024
Rollup merge of rust-lang#119657 - cls:slice_split_once-typo, r=ChrisDenton

Fix typo in docs for slice::split_once, slice::rsplit_once

This fixes a typo in the doc comments for these methods, which I tripped over while reading the docs: "If any matching elements are **resent** in the slice [...]", which is presumably meant to read **present**.

I mentioned this in rust-lang#112811, the tracking issue for `slice_split_once`, and was encouraged to open a PR.
@krh
Copy link

krh commented Jan 9, 2024

Should we include split_once_mut and rsplit_once_mut?

I was just reaching for split_once_mut and didn't find it, so I'd vote yes...

@Benjamin-L
Copy link
Contributor Author

Should we include split_once_mut and rsplit_once_mut?

I was just reaching for split_once_mut and didn't find it, so I'd vote yes...

I think adding this is probably a good idea, but if we're going to add slice::split_once_mut we should also add str::split_once_mut. If we do that, I'm assuming str::split_once_mut should be a separate feature from this one?

Either way, I'm gonna wait until we have a decision on whether return types should include the delimiter (#119799) before submitting a PR for the _mut variants.

@cls
Copy link
Contributor

cls commented Jan 10, 2024

I'm a newbie, but I thought I'd still stick my oar in. The change in #119799 seems fine, but I wondered if an alternative might be to leave returning the separator to a separate method.

In a vacuum that might seem clumsy, but my thinking is that, as @huonw said, returning the separator would be required for slice::split_once_mut, and I'm wondering whether that wouldn't also be true of str. If it would be then, if we left it to a separate method split_once_sep (or whatever), we could add the same method to str — at the same time as str::split_once_mut — which would then be more consistent between str::split_once and slice::split_once, but also internally within str, because then it wouldn't leave the question "why can I get a reference to the separator only for &mut str?".

However, this won't be an issue if the thinking is that str::split_once_mut would not return the separator either.

I don't know — food for thought.

@Benjamin-L
Copy link
Contributor Author

I like the split_once_sep proposal, for uniformity with the str API. Gonna mark #119799 as a draft and wait to see if anybody else has thoughts about this before trying to make any changes.

Another option might be to have something like str::split_inclusive, where the split element is included in the first return value. I think one of the arguments against that would be that the current api of slice::split_once ensures that the delimiter will always be a single element, which is not the case with str::pattern::Pattern. slice::split_once_inclusive would also be annoying to work with if you want a mutable reference to both the prefix and the delimiter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants