Add take_... functions to slices
#62282
Conversation
|
r? @Kimundi (rust_highfive has picked a reviewer for you, use r? to override) |
|
cc @nox |
|
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 |
|
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. |
|
|
|
ping from triage @cramertj can you update this with changes requested by Centril before I get someone to review it? thanks (also failing tests) |
|
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! |
|
ping from triage... hi @cramertj, can you please provide a status? |
|
I feel like all these functions should actually live as methods on |
|
|
||
| /// Takes the first element out of the slice. | ||
| /// | ||
| /// Returns a reference pointing to the first element of the old slice. |
There was a problem hiding this comment.
There is no such thing as an old slice, given there is not really a new slice either.
|
@nox This was addressed:
|
There is no need for |
|
Sorry, I misread your comment. |
No worries, I should have given more details as to why I think that anyway:
|
Hmm, kinda like how
I happen to think that that's how slices should have been implemented to begin with 🙂 It would make stuff like Whenever I needed any of these |
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.
A pair of pointers makes
The thing is, if there were those methods on |
|
Ping from triage: @cramertj any updates on this? |
|
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 Thanks |
That's fair enough, although earlier I wasn't able to come up with a benchmark that uses indexing and is faster on normal slices than on two-pointer slices. I'm guessing the binary ended up reusing the length rather than computing it over and over, which may actually be true most real world usages of slices.
I decided to give this a go in a programming exercise that required linear search in a part of the problem. I passed around slice iterators, and it worked pretty well until I decided to use binary search instead, which was quite a pain to change. I could have added a couple I might change my mind on this later, but right now I think it's good to limit If |
|
I believe I've addressed all outstanding comments, aside from the suggestion that the methods be moved to I've also removed the |
|
This comment has been minimized.
This comment has been minimized.
|
I think this is ready for review again. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
There was a problem hiding this comment.
Could you add a few (non doc) tests still? Thinking about testing these cases:
take[_mut]with in bound index (with all three range types)take[_mut]with out of bound index (with all three range types)take[_mut]withusize::MAXindex (you can create a[(); usize::MAX]array for that)take_[first|last][_mut]with non-empty slicetake_[first|last][_mut]with empty slicetake_last[_mut]with the[(); usize::MAX]array
With these tests and the docs for the first/last methods adjusted, I'm happy to merge this!
| /// Takes the first element out of the slice. | ||
| /// | ||
| /// Returns a reference pointing to the first element of the old slice. | ||
| /// | ||
| /// Returns `None` if the slice is empty. |
There was a problem hiding this comment.
Maybe we want to adopt the formulation from the take and take_mut docs for this and the three other methods below? I.e.:
Removes and returns the [first|last] element from the slice.
If the slice is empty,
Noneis returned and the slice is not modified.
This would also resolve @nox's remark.
There was a problem hiding this comment.
I'm a little confused about how to phrase this, because I want to make it clear that the element itself is staying in place, and the only thing being manipulated is what the slice refers to. That is, "Removes and returns the first element from the slice." sounds to me like it's taking an element out from the backing memory, which isn't the case. WDYT?
There was a problem hiding this comment.
Yeah, you're right, it's a bit misleading. I don't have a good idea, but maybe something like:
Returns a reference to the first element and modifies
selfto now start at the second element.
or
Returns a reference to the first element and modifies
selfto not include that element anymore.
or
Returns a reference to the first element and sets
selfto&self[1..].
In all cases, the first "self" could be replaced by "this slice". And in all cases, I would add as a second paragraph:
If the slice is empty,
Noneis returned and [self|the slice] is not modified. This function is essentially equivalent toself.take(1..), but returns&Tinstead of&[T].
And if we change the take_[first|last] versions of this method, we should probably also adjust the formulation for take[_mut]. It currently says "removes and returns" which, as you said, is misleading.
This adds the following associated functions to `[T]`: - take - take_mut - take_first - take_first_mut - take_last - take_last_mut
066f566
to
36b8293
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@cramertj Ping, the CI is red. Seems to be a simple rustfmt error. No hurries thought; just wanted to ping you in case you didn't see the CI failure. |
|
☔ The latest upstream changes (presumably #71483) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closing this due to inactivity. |
|
@cramertj Would love to get this merged some day. Just ping me if you have time again :) |
|
FWIW I've released |
Add slice take methods Revival of rust-lang#62282 This PR adds the following slice methods: - `take` - `take_mut` - `take_first` - `take_first_mut` - `take_last` - `take_last_mut` r? `@LukasKalbertodt`
This adds the following associated functions to
[T]: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
&&Selfrather than&Self, which isn't yet a stableSelftype. 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&&Selfmethod receivers-- it was just left out in order to keep the stable arbitrary_self_types surface small.