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

Add `take_...` functions to slices #62282

Open
wants to merge 1 commit into
base: master
from

Conversation

@cramertj
Copy link
Member

commented Jul 1, 2019

This adds the following associated functions to [T]:

  • take_front
  • take_front_mut
  • take_end
  • take_end_mut
  • take_first
  • take_first_mut
  • take_last
  • take_last_mut

A previous PR that would have added some of this functionality was closed due to inactivity: #49173

I personally use these functions fairly often, and I often feel guilty re-hand-rolling them.

One thing to note is that these are just associated functions, rather than methods, because they use &&Self rather than &Self, which isn't yet a stable Self type. It should be backwards-compatible to change this, however, so long as there aren't major method resolution conflicts or similar. AFAIK there aren't any technical blockers to stabilizing &&Self method receivers-- it was just left out in order to keep the stable arbitrary_self_types surface small.

Add `take_...` functions to slices
This adds the following associated functions to `[T]`:
- take_front
- take_front_mut
- take_end
- take_end_mut
- take_first
- take_first_mut
- take_last
- take_last_mut
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 1, 2019

r? @Kimundi

(rust_highfive has picked a reviewer for you, use r? to override)

@cramertj

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

cc @nox

#[inline]
#[unstable(feature = "slice_take", issue = "62280")]
fn take<'a>(slice: &mut &'a Self) -> &'a Self {
crate::mem::replace(slice, &[])

This comment has been minimized.

Copy link
@Centril

Centril Jul 1, 2019

Member
Suggested change
crate::mem::replace(slice, &[])
mem::take(slice)

(&[T] implements Default)

This comment has been minimized.

Copy link
@timvermeulen

timvermeulen Jul 2, 2019

Contributor

mem::take(slice) is so concise that there's probably no point in having Self::take(slice) anymore (I do realize that this method isn't public, so it doesn't really matter)

This comment has been minimized.

Copy link
@czipperz

czipperz Jul 2, 2019

Contributor

Well it does provide nice symmetry with take_mut. But that could be implemented using mem::take.

#[inline]
#[unstable(feature = "slice_take", issue = "62280")]
fn take_mut<'a>(slice: &mut &'a mut Self) -> &'a mut Self {
crate::mem::replace(slice, &mut [])

This comment has been minimized.

Copy link
@Centril

Centril Jul 1, 2019

Member
Suggested change
crate::mem::replace(slice, &mut [])
mem::take(slice)

/// Empty out a slice and return a pointer to its contents.
///
/// This is equivalent to `mem::replace(&mut slice, &[])`.

This comment has been minimized.

Copy link
@Centril

Centril Jul 1, 2019

Member
Suggested change
/// This is equivalent to `mem::replace(&mut slice, &[])`.
/// This is equivalent to `mem::take(&mut slice)`.

/// Empty out a mutable slice and return a pointer to its contents.
///
/// This is equivalent to `mem::replace(&mut slice, &mut [])`.

This comment has been minimized.

Copy link
@Centril

Centril Jul 1, 2019

Member
Suggested change
/// This is equivalent to `mem::replace(&mut slice, &mut [])`.
/// This is equivalent to `mem::take(&mut slice)`.

This comment has been minimized.

Copy link
@czipperz

czipperz Jul 2, 2019

Contributor

^take. Also, it's redundant and overwhelming for the author when reviewers go through and write the same comment.

This comment has been minimized.

Copy link
@Centril

Centril Jul 2, 2019

Member

Also, it's redundant and overwhelming for the author when reviewers go through and write the same comment.

The point of these suggestions is that they can be auto-applied and so you don't need to check out your branch and whatnot.

This comment has been minimized.

Copy link
@cramertj

cramertj Jul 2, 2019

Author Member

FWIW, I have never once autoapplied these suggestions-- I don't know any way to do that that doesn't create an extra commit for each one.

This comment has been minimized.

Copy link
@Centril

Centril Jul 2, 2019

Member

There's a button "Add suggestion to batch" to the right?

This comment has been minimized.

Copy link
@cramertj

cramertj Jul 2, 2019

Author Member

It's grayed out for me 🤷‍♀

This comment has been minimized.

Copy link
@Centril

Centril Jul 2, 2019

Member

@cramertj Try going to the "Files changed" tab. They are greyed out for me if I visit #62282 but not if I visit https://github.com/rust-lang/rust/pull/62282/files.

/// #![feature(slice_take)]
///
/// let mut slice: &[_] = &['a', 'b', 'c'];
/// let first_two = core::slice::take_front(&mut slice, 2).unwrap();

This comment has been minimized.

Copy link
@Centril

Centril Jul 1, 2019

Member
Suggested change
/// let first_two = core::slice::take_front(&mut slice, 2).unwrap();
/// let first_two = std::slice::take_front(&mut slice, 2).unwrap();
/// #![feature(slice_take)]
///
/// let mut slice: &mut [_] = &mut ['a', 'b', 'c'];
/// let mut last_two = core::slice::take_back_mut(&mut slice, 2).unwrap();

This comment has been minimized.

Copy link
@Centril

Centril Jul 1, 2019

Member
Suggested change
/// let mut last_two = core::slice::take_back_mut(&mut slice, 2).unwrap();
/// let mut last_two = std::slice::take_back_mut(&mut slice, 2).unwrap();
/// #![feature(slice_take)]
///
/// let mut slice: &[_] = &['a', 'b', 'c'];
/// let first = core::slice::take_first(&mut slice).unwrap();

This comment has been minimized.

Copy link
@Centril

Centril Jul 1, 2019

Member
Suggested change
/// let first = core::slice::take_first(&mut slice).unwrap();
/// let first = std::slice::take_first(&mut slice).unwrap();
/// #![feature(slice_take)]
///
/// let mut slice: &[_] = &['a', 'b', 'c'];
/// let first = core::slice::take_first_mut(&mut slice).unwrap();

This comment has been minimized.

Copy link
@Centril

Centril Jul 1, 2019

Member
Suggested change
/// let first = core::slice::take_first_mut(&mut slice).unwrap();
/// let first = std::slice::take_first_mut(&mut slice).unwrap();
/// #![feature(slice_take)]
///
/// let mut slice: &[_] = &['a', 'b', 'c'];
/// let last = core::slice::take_last(&mut slice).unwrap();

This comment has been minimized.

Copy link
@Centril

Centril Jul 1, 2019

Member
Suggested change
/// let last = core::slice::take_last(&mut slice).unwrap();
/// let last = std::slice::take_last(&mut slice).unwrap();
/// #![feature(slice_take)]
///
/// let mut slice: &[_] = &['a', 'b', 'c'];
/// let last = core::slice::take_last_mut(&mut slice).unwrap();

This comment has been minimized.

Copy link
@Centril

Centril Jul 1, 2019

Member
Suggested change
/// let last = core::slice::take_last_mut(&mut slice).unwrap();
/// let last = std::slice::take_last_mut(&mut slice).unwrap();
@timvermeulen

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

take_end forwards the len parameter directly to split_at, but take_end(slice, n) should take the last n elements (according to its docs), not take the suffix starting at index n. So it should probably either split the slice at slice.len() - len, or be documented to take an index.

I personally think having it take an index is more useful. In my limited experience with slices I more frequently want to take (or discard) the elements starting at a given index, rather than taking the last n elements. Admittedly, the current semantics do play nicely with take_last.

@cramertj

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2019

Ah, good call! I did intend for it to take at an index, but I got lazy with copy/pasting. Will fix, thanks for pointing that out. It is a somewhat interesting asymmetry, though.

@timvermeulen

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

take_end(3) does sound to me like taking the last three elements, maybe something like take_from would be less confusing?

@Dylan-DPC

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

ping from triage @cramertj can you update this with changes requested by Centril before I get someone to review it? thanks (also failing tests)

@cramertj

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2019

Sure-- there's one open design question above, but I'll at least make this PR self-consistent and address the other issues. Thanks for the ping!

@rholderfield

This comment has been minimized.

Copy link

commented Jul 26, 2019

ping from triage... hi @cramertj, can you please provide a status?

@nox

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

I feel like all these functions should actually live as methods on std::slice::Iter.


/// Takes the first element out of the slice.
///
/// Returns a reference pointing to the first element of the old slice.

This comment has been minimized.

Copy link
@nox

nox Jul 27, 2019

Contributor

There is no such thing as an old slice, given there is not really a new slice either.

@timvermeulen

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

@nox This was addressed:

One thing to note is that these are just associated functions, rather than methods, because they use &&Self rather than &Self, which isn't yet a stable Self type. It should be backwards-compatible to change this, however, so long as there aren't major method resolution conflicts or similar. AFAIK there aren't any technical blockers to stabilizing &&Self method receivers-- it was just left out in order to keep the stable arbitrary_self_types surface small.

@nox

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

One thing to note is that these are just associated functions, rather than methods, because they use &&Self rather than &Self, which isn't yet a stable Self type. It should be backwards-compatible to change this, however, so long as there aren't major method resolution conflicts or similar. AFAIK there aren't any technical blockers to stabilizing &&Self method receivers-- it was just left out in order to keep the stable arbitrary_self_types surface small.

There is no need for &&Self if the methods live on std::slice::Iter.

@timvermeulen

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

Sorry, I misread your comment.

@nox

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

Sorry, I misread your comment.

No worries, I should have given more details as to why I think that anyway:

  • Those methods all take a &mut, but slices are Copy, I could imagine badly using one of those methods in a way where you take the n first elements but the rest of the code continue to use the original slice instead of the mutated one because it was mistakenly copied somewhere.

  • Doing those operations on slices, you have to adjust the pointer and the length every time, whereas std::slice::Iter is implemented as a pair of start/end pointers, so you only need to readjust the start pointer.

@timvermeulen

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

@nox

  • Those methods all take a &mut, but slices are Copy, I could imagine badly using one of those methods in a way where you take the n first elements but the rest of the code continue to use the original slice instead of the mutated one because it was mistakenly copied somewhere.

Hmm, kinda like how Range isn't Copy because it's already an iterator? Do you have an example of when you might accidentally mutate a copy of the slice instead of the original slice? Calling any of these methods on an immutable slice variable will fortunately cause a compiler error rather than mutating a copy, so I'm not sure when this could cause any problems.

  • Doing those operations on slices, you have to adjust the pointer and the length every time, whereas std::slice::Iter is implemented as a pair of start/end pointers, so you only need to readjust the start pointer.

I happen to think that that's how slices should have been implemented to begin with 🙂 It would make stuff like split_at a lot simpler too in terms of the number of operations, and slice::Iter could then just wrap a slice without losing performance.

Whenever I needed any of these take_ methods, I don't think having them on slice::Iter would have been very useful for me. If it requires converting to an iterator and then back to a slice, you probably may as well call split_at instead.

@nox

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

Hmm, kinda like how Range isn't Copy because it's already an iterator? Do you have an example of when you might accidentally mutate a copy of the slice instead of the original slice? Calling any of these methods on an immutable slice variable will fortunately cause a compiler error rather than mutating a copy, so I'm not sure when this could cause any problems.

Forget about that, the cases I had in mind involved method calls on temporaries, but there are no method calls to begin with in this RFC.

I happen to think that that's how slices should have been implemented to begin with 🙂 It would make stuff like split_at a lot simpler too in terms of the number of operations, and slice::Iter could then just wrap a slice without losing performance.

A pair of pointers makes Index implementations more complicated though.

Whenever I needed any of these take_ methods, I don't think having them on slice::Iter would have been very useful for me. If it requires converting to an iterator and then back to a slice, you probably may as well call split_at instead.

The thing is, if there were those methods on slice::Iter, how often would you even need a slice again? I usually either need indexing, or extracting things out from the beginning of the slice, not both at once.

@JohnTitor

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

Ping from triage: @cramertj any updates on this?

@gagan0723

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

Second ping from triage, @cramertj any updates on this? All checks are failing.

Note: Thanks for the PR. This will be closed and marked as S-inactive-closed next week. Feel free to re-open when you have time.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.