Take 2: Add `take_...` functions to slices #77065

timvermeulen · 2020-09-22T17:44:41Z

Revival of #62282

This PR adds the following slice methods:

take
take_mut
take_first
take_first_mut
take_last
take_last_mut

The first commit is just the code of the previous PR. I removed some tests that hung compilation (see #77062), I can add them back once that's fixed. I also slightly reworded the doc comments as requested in the previous PR, let me know if that can be improved still.

r? @LukasKalbertodt cc @cramertj @nox

timvermeulen · 2020-09-22T19:24:58Z

Thanks! Fixed.

cramertj · 2020-09-28T19:41:10Z

Thanks so much for taking over this! Sorry for losing track of the other PR.

bors · 2020-09-28T20:40:54Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

bors · 2020-10-07T19:43:30Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

AnthonyMikh · 2020-10-08T08:25:42Z

library/core/src/slice/mod.rs

+/// index at which to split. Returns `None` if the split index would
+/// overflow `usize`.
+#[inline]
+fn take_split_point(range: impl OneSidedRange<usize>) -> Option<(bool, usize)> {


Wouldn't it be better to make take_split_point a method of OneSideRange and implement it for ranges? Not only it is type safe, it would also somewhat speed up debug builds since compiler wouldn't have to emit code for match and panic.

I believe type safety wise there's no difference since this function also only takes OneSidedRanges, but otherwise that sounds reasonable. It would mean that take_split_point becomes part of OneSidedRange's API, but that's probably fine since it's unstable anyway?

Hmm, take_split_point in its current form can't really be implemented for any OneSidedRange<T>, because of the checked_add calls. Any idea what kind of API we could add to OneSidedRange that offers the same functionality without being restricted to integers?

I don't think that this restriction can be a concern in practice since at the moment you can't index slice with ranges of something other than usizes anyway.

Sure, but it'd be a bit weird to implement OneSidedRange only for usize ranges?

If it bothers you that much, you can just name it OneSidedUsizeRange ¯\_(ツ)_/¯

I think it's best to leave this as a private function for now.

If this were to be a public API (even just unstable), it'd probably need a better return type, instead of just using bool to indicate the direction. As a private function, that's fine though.

AnthonyMikh · 2020-10-08T08:32:05Z

Wouldn't it be easier to implement take_{first, last}{, _mut} in terms of split_{first, last}{, _mut}?

bors · 2020-10-18T21:18:32Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

camelid · 2020-11-06T19:31:39Z

@timvermeulen rust-lang/rust follows a no-merge policy. Can you please rebase instead of merging master into your branch? You can use this command to rebase on master, which should also remove the merge commits:

$ git rebase $(git merge-base master HEAD)

Just make sure you git pull the latest upstream changes into master.

library/core/src/slice/mod.rs

m-ou-se

Sorry for the delay!

Looks great, just a few small comments:

m-ou-se · 2020-11-28T12:58:35Z

library/core/src/slice/mod.rs

+    /// Returns the subslice corresponding to the given range,
+    /// and modifies the slice to no longer include this subslice.
+    ///


I think it'd be good to note also here in this doc comment that this only accepts one-sided ranges like ..n or n.., but not i..j. (And if you feel like it: maybe even add a compile_fail example that shows that you can't just cut a slice from the middle with .take(5..10) or something.)

m-ou-se · 2020-11-28T13:18:13Z

library/core/tests/slice.rs

@@ -2038,3 +2038,83 @@ fn test_slice_run_destructors() {

    assert_eq!(x.get(), 1);
 }
+


In the previous PR, Lukas suggested some tests involving usize::MAX: #62282 (review)

Can you add those?

m-ou-se · 2020-11-28T13:20:58Z

library/core/src/slice/mod.rs

+    #[inline]
+    #[unstable(feature = "slice_take", issue = "62280")]
+    pub fn take<'a, R: OneSidedRange<usize>>(self: &mut &'a Self, range: R) -> Option<&'a Self> {


It'd probably good to mark take and take_mut as #[must_use], because slice.take(..10); silently does nothing if there were not enough elements. See #62282 (comment)

WaffleLapkin · 2020-11-30T22:23:21Z

library/core/src/slice/mod.rs

+        (Bound::Unbounded, Bound::Included(i)) => (true, i.checked_add(1)?),
+        (Bound::Excluded(i), Bound::Unbounded) => (false, i.checked_add(1)?),
+        (Bound::Included(i), Bound::Unbounded) => (false, *i),
+        _ => unreachable!(),


Wouldn't it be more convenient to add something like

// I'm bad at naming, but anyway enum Side<T> { From(Bound<T>), To(Bound<T>), } fn bound(&self) -> Bound<&T>;

to OneSidedRange<T>?

crlf0710 · 2020-12-18T11:32:46Z

@timvermeulen Ping from triage: Would you mind addressing the comments above? Thanks.

timvermeulen · 2021-01-01T18:25:55Z

@crlf0710 Thanks for the ping! I'll address them shortly.

JohnCSimon · 2021-01-19T06:01:17Z

@timvermeulen - Hi, I'm going to close this due to inactivity. Feel free to reopen if you have more commits.

rust-highfive assigned LukasKalbertodt Sep 22, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 22, 2020

This comment has been minimized.

Sign in to view

timvermeulen force-pushed the take_slice branch from 6ea397b to a7faf20 Compare September 22, 2020 19:24

AnthonyMikh reviewed Oct 8, 2020

View reviewed changes

camelid added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 6, 2020

camelid added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 6, 2020

cramertj and others added 6 commits November 6, 2020 22:11

Add take_... functions to slices

bd417a4

Remove hanging tests

23d74f2

Adjust wording

5d6272a

Avoid underflow

8dda5c4

Don't replace self by empty slice in slice::take

e35ba52

Implement take_first methods in terms of split_first methods

81b8edc

timvermeulen force-pushed the take_slice branch from 550703f to 81b8edc Compare November 6, 2020 21:13

AnthonyMikh reviewed Nov 6, 2020

View reviewed changes

library/core/src/slice/mod.rs Show resolved Hide resolved

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

m-ou-se assigned m-ou-se and unassigned LukasKalbertodt Nov 12, 2020

m-ou-se mentioned this pull request Nov 28, 2020

Tracking issue for slice_take #62280

Open

6 tasks

m-ou-se reviewed Nov 28, 2020

View reviewed changes

m-ou-se 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 Nov 28, 2020

WaffleLapkin reviewed Nov 30, 2020

View reviewed changes

JohnCSimon added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jan 19, 2021

JohnCSimon closed this Jan 19, 2021

WaffleLapkin mentioned this pull request Oct 12, 2021

Add slice take methods #88502

Merged

timvermeulen deleted the take_slice branch July 8, 2022 22:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take 2: Add `take_...` functions to slices #77065

Take 2: Add `take_...` functions to slices #77065

timvermeulen commented Sep 22, 2020

This comment has been minimized.

timvermeulen commented Sep 22, 2020

cramertj commented Sep 28, 2020

bors commented Sep 28, 2020

bors commented Oct 7, 2020

AnthonyMikh Oct 8, 2020

timvermeulen Oct 8, 2020

timvermeulen Oct 21, 2020

AnthonyMikh Nov 6, 2020 •

edited

Loading

timvermeulen Nov 6, 2020

AnthonyMikh Nov 7, 2020

m-ou-se Nov 28, 2020

AnthonyMikh commented Oct 8, 2020

bors commented Oct 18, 2020

camelid commented Nov 6, 2020

m-ou-se left a comment

m-ou-se Nov 28, 2020

m-ou-se Nov 28, 2020

m-ou-se Nov 28, 2020

WaffleLapkin Nov 30, 2020

crlf0710 commented Dec 18, 2020

timvermeulen commented Jan 1, 2021

JohnCSimon commented Jan 19, 2021

		@@ -2038,3 +2038,83 @@ fn test_slice_run_destructors() {

		assert_eq!(x.get(), 1);
		}

Take 2: Add take_... functions to slices #77065

Take 2: Add take_... functions to slices #77065

Conversation

timvermeulen commented Sep 22, 2020

This comment has been minimized.

timvermeulen commented Sep 22, 2020

cramertj commented Sep 28, 2020

bors commented Sep 28, 2020

bors commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyMikh Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnthonyMikh commented Oct 8, 2020

bors commented Oct 18, 2020

camelid commented Nov 6, 2020

m-ou-se left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crlf0710 commented Dec 18, 2020

timvermeulen commented Jan 1, 2021

JohnCSimon commented Jan 19, 2021

Take 2: Add `take_...` functions to slices #77065

Take 2: Add `take_...` functions to slices #77065

AnthonyMikh Nov 6, 2020 •

edited

Loading