Introduce <&[_]>::split_off and <&str>::split_off #49173

nox · 2018-03-19T13:46:40Z

No description provided.

rust-highfive · 2018-03-19T13:46:45Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @withoutboats (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Centril · 2018-03-19T14:45:56Z

This seems nice to have 👍
I'm kinda saddened that libcore can't use these (and existing methods in the same impl..) -- hopefully we can fix that someday.

cramertj · 2018-03-19T15:57:08Z

+1-- I've written these a handful of times.

I'd also like split_off_front and split_off_front_mut.

nox · 2018-03-19T15:59:09Z

@cramertj I plan to make a separate PR for a more general "cutting" concept, with "anchored ranges". I'll probably do that at the end of the week.

https://play.rust-lang.org/?gist=54e0077190f33596d9f8fa705bed0a66&version=nightly

Centril · 2018-03-19T16:39:01Z

src/libcore/str/mod.rs

+
+    #[inline]
+    fn split_off_mut<'a>(self: &mut &'a mut Self, at: usize) -> &'a mut str {
+        let (rest, split) = mem::replace(self, unsafe { from_utf8_unchecked_mut(&mut []) })


This is trivially safe since an empty string slice is UTF-8 by definition, but a comment to that effect would be nice here ;)
(Generally I think all uses of unsafe elimination blocks should come with a comment explaining the reason why it is safe.)

I finally added the damn comment. :3

cramertj · 2018-03-28T14:13:03Z

ping @withoutboats

nox · 2018-04-05T10:17:10Z

@withoutboats Ping.

SimonSapin · 2018-04-09T08:20:20Z

The implementation looks fine to me but the self: &mut &'a Self parameter is unusual. @rust-lang/libs what do you think?

alexcrichton · 2018-04-09T13:49:56Z

The method name here could be shadowed by Vec::split_off so we may want to rename?

We've also got methods like <[_]>::split_{first,last}, so I wonder if we'd want to provide similar treatment to those methods as well? (aka have all the split_* methods with a &mut &mut [T] version)

bors · 2018-04-12T02:59:20Z

☔ The latest upstream changes (presumably #49698) made this pull request unmergeable. Please resolve the merge conflicts.

pietroalbini · 2018-04-16T11:00:34Z

Ping from triage! Can @withoutboats (or someone else from @rust-lang/libs) review this?

withoutboats · 2018-04-16T11:07:22Z

Code looks good, I agree with Alex that the potential shadowing with the Vec API seems bad and it would be good to rename.

In regard to @SimonSapin's point about the return type, I think this is just an example of the kinds of new APIs arbitrary_self_types enables.

nox · 2018-04-20T13:53:42Z

What should I rename them to?

Centril · 2018-04-20T14:00:42Z

@nox How about: steal_from or take_from? This hints at the fact that the return value "steals" or takes something from the receiver starting from at and leaves the rest in place.

nox · 2018-04-20T14:31:33Z

I would expect such a method to set self to the rest of the slice, not the beginning.

TimNN · 2018-04-24T16:52:39Z

@nox / @alexcrichton: It looks like the main blocker here is deciding on names for the new methods. How do you want to move forward with this?

If this does essentially the same as existing split_off methods, I think it should be named similarly. If shadowing with Vec is a concern, maybe split_off_slice? Or keep split_off for now and discuss again prior to stabilization?

alexcrichton · 2018-04-24T17:36:50Z

I don't have a particular personal preference on name, but I do think it just needs to be different

pietroalbini · 2018-04-30T11:24:12Z

Ping from triage! What's the status of this? S-waiting-on-bikeshed?

alexcrichton · 2018-04-30T14:00:55Z

Indeed!

pietroalbini · 2018-04-30T14:08:42Z

Sure.

cramertj · 2018-04-30T15:40:11Z

How about split_tail or take_tail?

pietroalbini · 2018-06-04T10:19:25Z

Ping from triage @nox @alexcrichton! What we should do with this PR?

Centril · 2018-06-04T11:10:57Z

I propose that we just pick one of the suboptimal names proposed for now and optimize it during stabilization; maybe we'll have better ideas by then?

Centril · 2018-06-04T11:23:23Z

Also; I noticed that impl Default for &'_ mut str so the unsafe block can/should be replaced with a call to default.

shepmaster · 2018-06-04T13:45:44Z

pick one of the suboptimal names proposed for now and optimize it during stabilization; maybe we'll have better ideas by then

Can we say that explicitly in the docs somehow? "If you don't like this name, pop on over to #XXXXX and paint the bikeshed"

Centril · 2018-06-04T13:47:23Z

@shepmaster SGTM 👍

alexcrichton · 2018-06-27T14:09:35Z

Looks like this hasn't been updated in awhile so I'm going to close, but feel free to resubmit of course! I think @Centril's idea above makes sense to me too!

nox · 2019-07-26T11:59:31Z

I completely forgot this PR. I'm currently doing a crate for slices represented as a pair of start and end pointers (like slice::Iter) and I'm thinking that split_off should rather live on slice::Iter, given its representation fits the use case better.

cramertj · 2019-07-26T16:13:20Z

@nox I started on a revival of it in #62282

rust-highfive assigned withoutboats Mar 19, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 19, 2018

kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 19, 2018

Centril reviewed Mar 19, 2018

View reviewed changes

shepmaster added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 23, 2018

Introduce <&[_]>::split_off and <&str>::split_off

14191c8

nox force-pushed the split-off branch from 91e28b1 to 14191c8 Compare March 28, 2018 11:15

SimonSapin added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 6, 2018

pietroalbini added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 30, 2018

pietroalbini added the S-waiting-on-bikeshed Status: Awaiting a decision on trivial things. label Apr 30, 2018

shepmaster removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 6, 2018

alexcrichton closed this Jun 27, 2018

cramertj mentioned this pull request Jul 1, 2019

Add take_... functions to slices #62282

Closed

WaffleLapkin mentioned this pull request Oct 12, 2021

Add slice take methods #88502

Merged

dtolnay mentioned this pull request Dec 1, 2021

Tracking issue for slice_take #62280

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce <&[_]>::split_off and <&str>::split_off #49173

Introduce <&[_]>::split_off and <&str>::split_off #49173

nox commented Mar 19, 2018

rust-highfive commented Mar 19, 2018

Centril commented Mar 19, 2018

cramertj commented Mar 19, 2018 •

edited

Loading

nox commented Mar 19, 2018

Centril Mar 19, 2018

nox Mar 28, 2018

cramertj commented Mar 28, 2018

nox commented Apr 5, 2018

SimonSapin commented Apr 9, 2018

alexcrichton commented Apr 9, 2018

bors commented Apr 12, 2018

pietroalbini commented Apr 16, 2018

withoutboats commented Apr 16, 2018

nox commented Apr 20, 2018

Centril commented Apr 20, 2018

nox commented Apr 20, 2018

TimNN commented Apr 24, 2018

alexcrichton commented Apr 24, 2018

pietroalbini commented Apr 30, 2018

alexcrichton commented Apr 30, 2018

pietroalbini commented Apr 30, 2018

cramertj commented Apr 30, 2018 •

edited

Loading

pietroalbini commented Jun 4, 2018

Centril commented Jun 4, 2018

Centril commented Jun 4, 2018

shepmaster commented Jun 4, 2018

Centril commented Jun 4, 2018

alexcrichton commented Jun 27, 2018

nox commented Jul 26, 2019

cramertj commented Jul 26, 2019

Introduce <&[_]>::split_off and <&str>::split_off #49173

Introduce <&[_]>::split_off and <&str>::split_off #49173

Conversation

nox commented Mar 19, 2018

rust-highfive commented Mar 19, 2018

Centril commented Mar 19, 2018

cramertj commented Mar 19, 2018 • edited Loading

nox commented Mar 19, 2018

Centril Mar 19, 2018

Choose a reason for hiding this comment

nox Mar 28, 2018

Choose a reason for hiding this comment

cramertj commented Mar 28, 2018

nox commented Apr 5, 2018

SimonSapin commented Apr 9, 2018

alexcrichton commented Apr 9, 2018

bors commented Apr 12, 2018

pietroalbini commented Apr 16, 2018

withoutboats commented Apr 16, 2018

nox commented Apr 20, 2018

Centril commented Apr 20, 2018

nox commented Apr 20, 2018

TimNN commented Apr 24, 2018

alexcrichton commented Apr 24, 2018

pietroalbini commented Apr 30, 2018

alexcrichton commented Apr 30, 2018

pietroalbini commented Apr 30, 2018

cramertj commented Apr 30, 2018 • edited Loading

pietroalbini commented Jun 4, 2018

Centril commented Jun 4, 2018

Centril commented Jun 4, 2018

shepmaster commented Jun 4, 2018

Centril commented Jun 4, 2018

alexcrichton commented Jun 27, 2018

nox commented Jul 26, 2019

cramertj commented Jul 26, 2019

cramertj commented Mar 19, 2018 •

edited

Loading

cramertj commented Apr 30, 2018 •

edited

Loading