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 split_at_checked #119128

Closed
2 of 4 tasks
mina86 opened this issue Dec 19, 2023 · 9 comments · Fixed by #124678
Closed
2 of 4 tasks

Tracking Issue for split_at_checked #119128

mina86 opened this issue Dec 19, 2023 · 9 comments · Fixed by #124678
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@mina86
Copy link
Contributor

mina86 commented Dec 19, 2023

Feature gate: #![feature(split_at_checked)].

This is a tracking issue for the addition of split_at_checked and split_at_mut_checked methods to [T] and str types which are non-panicking versions of split_at and split_at_mut methods. Rather than panicking when spit index is out of range (like split_at does), the methods return None.

Public API

impl<T> [T] {
    pub fn split_at_checked(&self, mid: usize) -> Option<(&[T], &[T])>;
    pub fn split_at_mut_checked(&self, mid: usize) -> Option<(&[T], &[T])>;
}

impl str {
    pub fn split_at_checked(&self, mid: usize) -> Option<(&str, &str)>;
    pub fn split_at_mut_checked(&mut self, mid: usize) -> Option<(&str, &str)>;
}

Steps / History

Unresolved Questions

Name of the methods. Some possibilities (suggested in ACP discussion):

  1. split_at_checked and split_at_mut_checked — follows naming of split_at_unchecked and split_at_mut_unchecked. Using suffix makes the names sort nicely together.
  2. checked_split_at and checked_split_at_mut — follows naming convention of arithmetic types (e.g. checked_add). Those new functions serve similar purpose as checked arithmetic operations.
  3. try_split_at and try_split_at_mut — follows naming of various fallible methods such as try_from, try_new, try_for_each etc. Shortest of the three suggestions.

Footnotes

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

@mina86 mina86 added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 19, 2023
@tgross35
Copy link
Contributor

I wonder if the str method should return a Result since they have two failure modes - invalid char boundary and OOB of the string.

@mina86
Copy link
Contributor Author

mina86 commented Jan 10, 2024

Personally I’d rather not complicate the interface and have the methods on slice and on str return the same kind of value. No strong feelings though.

bors added a commit to rust-lang-ci/rust that referenced this issue Jan 22, 2024
core: introduce split_at{,_mut}_checked

Introduce split_at_checked and split_at_mut_checked methods to slices
types (including str) which are non-panicking versions of split_at and
split_at_mut  respectively.  This is analogous to get method being
non-panicking version of indexing.

- rust-lang/libs-team#308
- rust-lang#119128
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 22, 2024
core: introduce split_at{,_mut}_checked

Introduce split_at_checked and split_at_mut_checked methods to slices
types (including str) which are non-panicking versions of split_at and
split_at_mut  respectively.  This is analogous to get method being
non-panicking version of indexing.

- rust-lang/libs-team#308
- rust-lang#119128
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 22, 2024
core: introduce split_at{,_mut}_checked

Introduce split_at_checked and split_at_mut_checked methods to slices
types (including str) which are non-panicking versions of split_at and
split_at_mut  respectively.  This is analogous to get method being
non-panicking version of indexing.

- rust-lang/libs-team#308
- rust-lang#119128
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 22, 2024
core: introduce split_at{,_mut}_checked

Introduce split_at_checked and split_at_mut_checked methods to slices
types (including str) which are non-panicking versions of split_at and
split_at_mut  respectively.  This is analogous to get method being
non-panicking version of indexing.

- rust-lang/libs-team#308
- rust-lang#119128
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 22, 2024
Rollup merge of rust-lang#118578 - mina86:c, r=dtolnay

core: introduce split_at{,_mut}_checked

Introduce split_at_checked and split_at_mut_checked methods to slices
types (including str) which are non-panicking versions of split_at and
split_at_mut  respectively.  This is analogous to get method being
non-panicking version of indexing.

- rust-lang/libs-team#308
- rust-lang#119128
@faern
Copy link
Contributor

faern commented Feb 20, 2024

Thanks for pushing for this feature ❤️ I really want this to land! Panic free handling of byte slices has been in a pretty bad state a long time. I came here just now because I was trying out the new split_first_chunk just to realize I could not make the code improvements I have been wanting a long time without also having panic free slice splitting with a runtime mid. split_first_chunk and split_at_checked do the same thing really, but one of them have the splitting index as a const generic.

I just tried slice::split_first_chunk and it does wonders to my code. Both in terms of code simplicity/readability but also to amount of potentially panic locations.

@mina86 You can check the implementation PR step in your initial comment. That's merged 🥳

@UserIsntAvailable
Copy link
Contributor

UserIsntAvailable commented Apr 22, 2024

@m-ou-se Can we get a fcp for this? slice::split_at_{mut}_unchecked is getting stabilized on 1.79.0 and I think is going to be kinda akward to not also provide the safe non-panic option as well on the same release.

Thanks in advance.


Name of the methods. Some possibilities

For consistency with the existing methods on slice and str, it probably should be called split_at_{mut}_checked.

I wonder if the str method should return a Result since they have two failure modes - invalid char boundary and OOB of the string.

IMO returning option here should be fine. But t-libs might have something else to say about this?

@m-ou-se m-ou-se added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Apr 22, 2024
@Amanieu
Copy link
Member

Amanieu commented Apr 23, 2024

We discussed this in the libs-api meeting and are happy to merge this as it is. The _checked suffix is consistent with the already stabilized _unchecked methods, and returning an Option is consistent with the .get() method.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Apr 23, 2024

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

No concerns currently listed.

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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 23, 2024
@Amanieu Amanieu removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Apr 23, 2024
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Apr 23, 2024
@rfcbot
Copy link

rfcbot commented Apr 23, 2024

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

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 3, 2024
@rfcbot
Copy link

rfcbot commented May 3, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label May 3, 2024
compiler-errors added a commit to compiler-errors/rust that referenced this issue May 4, 2024
…plit-at-checked, r=jhpratt

Stabilize `split_at_checked`

Closes rust-lang#119128

For the const version of `slice::split_at_mut_checked`, I'm reusing the `const_slice_split_at_mut` feature flag (rust-lang#101804). I don't if it okay to reuse tracking issues or if it preferred to create new ones...
@bors bors closed this as completed in 93ca906 May 4, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue May 4, 2024
Rollup merge of rust-lang#124678 - UserIsntAvailable:feat/stabilize-split-at-checked, r=jhpratt

Stabilize `split_at_checked`

Closes rust-lang#119128

For the const version of `slice::split_at_mut_checked`, I'm reusing the `const_slice_split_at_mut` feature flag (rust-lang#101804). I don't if it okay to reuse tracking issues or if it preferred to create new ones...
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 10, 2024
@Stargateur
Copy link
Contributor

Stargateur commented Oct 11, 2024

this was duplicate of #90410. I would have prefer Result.

This become stable very fast Oo. (nice)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants