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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions library/core/src/ops/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,9 @@ pub use self::range::{Range, RangeFrom, RangeFull, RangeTo};
#[stable(feature = "inclusive_range", since = "1.26.0")]
pub use self::range::{Bound, RangeBounds, RangeInclusive, RangeToInclusive};

#[unstable(feature = "one_sided_range", issue = "69780")]
pub use self::range::OneSidedRange;

#[unstable(feature = "try_trait", issue = "42327")]
pub use self::r#try::Try;

Expand Down
18 changes: 18 additions & 0 deletions library/core/src/ops/range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1004,3 +1004,21 @@ impl<T> RangeBounds<T> for RangeToInclusive<&T> {
Included(self.end)
}
}

/// `OneSidedRange` is implemented by Rust's built-in range types which
/// are unbounded on one side. For example, `a..`, `..b` and `..=c` implement
/// `OneSidedRange`, but `..`, `d..e`, and `f..=g` do not.
///
/// Types which implement `OneSidedRange<T>` must return `Bound::Unbounded`
/// from exactly one of `RangeBounds::start_bound` and `RangeBounds::end_bound`.
#[unstable(feature = "one_sided_range", issue = "69780")]
pub trait OneSidedRange<T: ?Sized>: RangeBounds<T> {}

#[unstable(feature = "one_sided_range", issue = "69780")]
impl<T> OneSidedRange<T> for RangeTo<T> where Self: RangeBounds<T> {}

#[unstable(feature = "one_sided_range", issue = "69780")]
impl<T> OneSidedRange<T> for RangeFrom<T> where Self: RangeBounds<T> {}

#[unstable(feature = "one_sided_range", issue = "69780")]
impl<T> OneSidedRange<T> for RangeToInclusive<T> where Self: RangeBounds<T> {}
246 changes: 245 additions & 1 deletion library/core/src/slice/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use crate::cmp::Ordering::{self, Equal, Greater, Less};
use crate::marker::Copy;
use crate::mem;
use crate::num::NonZeroUsize;
use crate::ops::{FnMut, Range, RangeBounds};
use crate::ops::{Bound, FnMut, OneSidedRange, Range, RangeBounds};
use crate::option::Option;
use crate::option::Option::{None, Some};
use crate::ptr;
Expand Down Expand Up @@ -73,6 +73,24 @@ pub use sort::heapsort;
#[stable(feature = "slice_get_slice", since = "1.28.0")]
pub use index::SliceIndex;

/// Calculates the direction and split point of a one-sided range.
///
/// Helper for `take` and `take_mut` which returns a boolean
/// indicating whether the front of the split is being taken
/// (as opposed to the back), as well as a number indicating the
/// 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.

Some(match (range.start_bound(), range.end_bound()) {
(Bound::Unbounded, Bound::Excluded(i)) => (true, *i),
(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>?

})
}

#[lang = "slice"]
#[cfg(not(test))]
impl<T> [T] {
Expand Down Expand Up @@ -3169,6 +3187,232 @@ impl<T> [T] {

left
}

/// Returns the subslice corresponding to the given range,
/// and modifies the slice to no longer include this subslice.
///
Comment on lines +3191 to +3193
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.)

/// Returns `None` and does not modify the slice if the given
/// range is out of bounds.
///
/// # Examples
///
/// Taking the first three items from a slice (via `..3`):
///
/// ```
/// #![feature(slice_take)]
///
/// let mut slice: &[_] = &['a', 'b', 'c', 'd'];
/// let mut first_three = slice.take(..3).unwrap();
///
/// assert_eq!(slice, &['d']);
/// assert_eq!(first_three, &['a', 'b', 'c']);
/// ```
///
/// Taking the tail of a slice starting at index two (via `2..`):
///
/// ```
/// #![feature(slice_take)]
///
/// let mut slice: &[_] = &['a', 'b', 'c', 'd'];
/// let mut tail = slice.take(2..).unwrap();
///
/// assert_eq!(slice, &['a', 'b']);
/// assert_eq!(tail, &['c', 'd']);
/// ```
///
/// Getting `None` when `range` starts or ends outside of the slice:
///
/// ```
/// #![feature(slice_take)]
///
/// let mut slice: &[_] = &['a', 'b', 'c', 'd'];
///
/// assert_eq!(None, slice.take(5..));
/// assert_eq!(None, slice.take(..5));
/// assert_eq!(None, slice.take(..=4));
/// let expected: &[char] = &['a', 'b', 'c', 'd'];
/// assert_eq!(Some(expected), slice.take(..4));
/// ```
#[inline]
#[unstable(feature = "slice_take", issue = "62280")]
pub fn take<'a, R: OneSidedRange<usize>>(self: &mut &'a Self, range: R) -> Option<&'a Self> {
Comment on lines +3236 to +3238
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)

let (taking_front, split_index) = take_split_point(range)?;
if split_index > self.len() {
return None;
}
let (front, back) = self.split_at(split_index);
if taking_front {
*self = back;
Some(front)
} else {
*self = front;
Some(back)
}
}

/// Returns the mutable subslice corresponding to the given range,
/// and modifies the slice to no longer include this subslice.
///
/// Returns `None` and does not modify the slice if the given
/// range is out of bounds.
///
/// # Examples
///
/// Taking the first three items from a slice (via `..3`):
///
/// ```
/// #![feature(slice_take)]
///
/// let mut slice: &mut [_] = &mut ['a', 'b', 'c', 'd'];
/// let mut first_three = slice.take_mut(..3).unwrap();
///
/// assert_eq!(slice, &mut ['d']);
/// assert_eq!(first_three, &mut ['a', 'b', 'c']);
/// ```
///
/// Taking the tail of a slice starting at index two (via `2..`):
///
/// ```
/// #![feature(slice_take)]
///
/// let mut slice: &mut [_] = &mut ['a', 'b', 'c', 'd'];
/// let mut tail = slice.take_mut(2..).unwrap();
///
/// assert_eq!(slice, &mut ['a', 'b']);
/// assert_eq!(tail, &mut ['c', 'd']);
/// ```
///
/// Getting `None` when `range` starts or ends outside of the slice:
///
/// ```
/// #![feature(slice_take)]
///
/// let mut slice: &mut [_] = &mut ['a', 'b', 'c', 'd'];
///
/// assert_eq!(None, slice.take_mut(5..));
/// assert_eq!(None, slice.take_mut(..5));
/// assert_eq!(None, slice.take_mut(..=4));
/// let expected: &mut [_] = &mut ['a', 'b', 'c', 'd'];
/// assert_eq!(Some(expected), slice.take_mut(..4));
/// ```
#[inline]
#[unstable(feature = "slice_take", issue = "62280")]
pub fn take_mut<'a, R: OneSidedRange<usize>>(
self: &mut &'a mut Self,
range: R,
) -> Option<&'a mut Self> {
let (taking_front, split_index) = take_split_point(range)?;
if split_index > self.len() {
return None;
}
let original = crate::mem::take(self);
let (front, back) = original.split_at_mut(split_index);
if taking_front {
*self = back;
Some(front)
} else {
*self = front;
Some(back)
}
}

/// Returns a reference to the first element of the slice,
/// and modifies the slice to no longer include this element.
///
/// Returns `None` if the slice is empty.
///
/// # Examples
///
/// ```
/// #![feature(slice_take)]
///
/// let mut slice: &[_] = &['a', 'b', 'c'];
/// let first = slice.take_first().unwrap();
///
/// assert_eq!(slice, &['b', 'c']);
/// assert_eq!(first, &'a');
/// ```
#[inline]
#[unstable(feature = "slice_take", issue = "62280")]
pub fn take_first<'a>(self: &mut &'a Self) -> Option<&'a T> {
let (first, rem) = self.split_first()?;
*self = rem;
Some(first)
}

/// Returns a mutable reference to the first element of the slice,
/// and modifies the slice to no longer include this element.
///
/// Returns `None` if the slice is empty.
///
/// # Examples
///
/// ```
/// #![feature(slice_take)]
///
/// let mut slice: &mut [_] = &mut ['a', 'b', 'c'];
/// let first = slice.take_first_mut().unwrap();
/// *first = 'd';
///
/// assert_eq!(slice, &['b', 'c']);
/// assert_eq!(first, &'d');
/// ```
#[inline]
#[unstable(feature = "slice_take", issue = "62280")]
pub fn take_first_mut<'a>(self: &mut &'a mut Self) -> Option<&'a mut T> {
let (first, rem) = mem::take(self).split_first_mut()?;
m-ou-se marked this conversation as resolved.
Show resolved Hide resolved
*self = rem;
Some(first)
}

/// Returns a reference to the last element of the slice,
/// and modifies the slice to no longer include this element.
///
/// Returns `None` if the slice is empty.
///
/// # Examples
///
/// ```
/// #![feature(slice_take)]
///
/// let mut slice: &[_] = &['a', 'b', 'c'];
/// let last = slice.take_last().unwrap();
///
/// assert_eq!(slice, &['a', 'b']);
/// assert_eq!(last, &'c');
/// ```
#[inline]
#[unstable(feature = "slice_take", issue = "62280")]
pub fn take_last<'a>(self: &mut &'a Self) -> Option<&'a T> {
let (last, rem) = self.split_last()?;
*self = rem;
Some(last)
}

/// Returns a mutable reference to the last element of the slice,
/// and modifies the slice to no longer include this element.
///
/// Returns `None` if the slice is empty.
///
/// # Examples
///
/// ```
/// #![feature(slice_take)]
///
/// let mut slice: &mut [_] = &mut ['a', 'b', 'c'];
/// let last = slice.take_last_mut().unwrap();
/// *last = 'd';
///
/// assert_eq!(slice, &['a', 'b']);
/// assert_eq!(last, &'d');
/// ```
#[inline]
#[unstable(feature = "slice_take", issue = "62280")]
pub fn take_last_mut<'a>(self: &mut &'a mut Self) -> Option<&'a mut T> {
let (last, rem) = mem::take(self).split_last_mut()?;
*self = rem;
Some(last)
}
}

#[stable(feature = "rust1", since = "1.0.0")]
Expand Down
1 change: 1 addition & 0 deletions library/core/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
#![feature(try_trait)]
#![feature(slice_internals)]
#![feature(slice_partition_dedup)]
#![feature(slice_take)]
#![feature(int_error_matching)]
#![feature(array_value_iter)]
#![feature(iter_advance_by)]
Expand Down
80 changes: 80 additions & 0 deletions library/core/tests/slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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?

macro_rules! take_tests {
(slice: &$slice:expr, $($tts:tt)*) => {
take_tests!(ty: &[_], slice: &$slice, $($tts)*);
};
(slice: &mut $slice:expr, $($tts:tt)*) => {
take_tests!(ty: &mut [_], slice: &mut $slice, $($tts)*);
};
(ty: $ty:ty, slice: $slice:expr, method: $method:ident, $(($test_name:ident, ($($args:expr),*), $output:expr, $remaining:expr),)*) => {
$(
#[test]
fn $test_name() {
let mut slice: $ty = $slice;
assert_eq!($output, slice.$method($($args)*));
let remaining: $ty = $remaining;
assert_eq!(remaining, slice);
}
)*
};
}

take_tests! {
slice: &[0, 1, 2, 3], method: take,
(take_in_bounds_range_to, (..1), Some(&[0] as _), &[1, 2, 3]),
(take_in_bounds_range_to_inclusive, (..=0), Some(&[0] as _), &[1, 2, 3]),
(take_in_bounds_range_from, (2..), Some(&[2, 3] as _), &[0, 1]),
(take_oob_range_to, (..5), None, &[0, 1, 2, 3]),
(take_oob_range_to_inclusive, (..=4), None, &[0, 1, 2, 3]),
(take_oob_range_from, (5..), None, &[0, 1, 2, 3]),
}

take_tests! {
slice: &mut [0, 1, 2, 3], method: take_mut,
(take_mut_in_bounds_range_to, (..1), Some(&mut [0] as _), &mut [1, 2, 3]),
(take_mut_in_bounds_range_to_inclusive, (..=0), Some(&mut [0] as _), &mut [1, 2, 3]),
(take_mut_in_bounds_range_from, (2..), Some(&mut [2, 3] as _), &mut [0, 1]),
(take_mut_oob_range_to, (..5), None, &mut [0, 1, 2, 3]),
(take_mut_oob_range_to_inclusive, (..=4), None, &mut [0, 1, 2, 3]),
(take_mut_oob_range_from, (5..), None, &mut [0, 1, 2, 3]),
}

take_tests! {
ty: &[_], slice: &[1, 2], method: take_first,
(take_first_nonempty, (), Some(&1), &[2]),
}

take_tests! {
ty: &mut [_], slice: &mut [1, 2], method: take_first_mut,
(take_first_mut_nonempty, (), Some(&mut 1), &mut [2]),
}

take_tests! {
ty: &[_], slice: &[1, 2], method: take_last,
(take_last_nonempty, (), Some(&2), &[1]),
}

take_tests! {
ty: &mut [_], slice: &mut [1, 2], method: take_last_mut,
(take_last_mut_nonempty, (), Some(&mut 2), &mut [1]),
}

take_tests! {
ty: &[()], slice: &[], method: take_first,
(take_first_empty, (), None, &[]),
}

take_tests! {
ty: &mut [()], slice: &mut [], method: take_first_mut,
(take_first_mut_empty, (), None, &mut []),
}

take_tests! {
ty: &[()], slice: &[], method: take_last,
(take_last_empty, (), None, &[]),
}

take_tests! {
ty: &mut [()], slice: &mut [], method: take_last_mut,
(take_last_mut_empty, (), None, &mut []),
}