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

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

Closed
wants to merge 6 commits into from

Conversation

timvermeulen
Copy link
Contributor

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

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

This comment has been minimized.

@timvermeulen
Copy link
Contributor Author

Thanks! Fixed.

@cramertj
Copy link
Member

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

@bors
Copy link
Contributor

bors commented Sep 28, 2020

☔ 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
Copy link
Contributor

bors commented Oct 7, 2020

☔ 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

/// 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)> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@AnthonyMikh AnthonyMikh Nov 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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

@bors
Copy link
Contributor

bors commented Oct 18, 2020

☔ 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 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
Copy link
Member

camelid commented Nov 6, 2020

@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.

@camelid 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
@camelid 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 m-ou-se assigned m-ou-se and unassigned LukasKalbertodt Nov 12, 2020
@m-ou-se m-ou-se mentioned this pull request Nov 28, 2020
6 tasks
Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay!

Looks great, just a few small comments:

Comment on lines +3191 to +3193
/// Returns the subslice corresponding to the given range,
/// and modifies the slice to no longer include this subslice.
///
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

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

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Can you add those?

Comment on lines +3236 to +3238
#[inline]
#[unstable(feature = "slice_take", issue = "62280")]
pub fn take<'a, R: OneSidedRange<usize>>(self: &mut &'a Self, range: R) -> Option<&'a Self> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@m-ou-se 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
(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!(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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

@timvermeulen
Copy link
Contributor Author

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

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

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

@JohnCSimon JohnCSimon closed this Jan 19, 2021
@timvermeulen timvermeulen deleted the take_slice branch July 8, 2022 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.