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 SliceIndex impl for pair of Bounds #81108

Closed
wants to merge 1 commit into from
Closed
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
103 changes: 103 additions & 0 deletions library/core/src/slice/index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ mod private_slice_index {
impl Sealed for ops::RangeInclusive<usize> {}
#[stable(feature = "slice_get_slice", since = "1.28.0")]
impl Sealed for ops::RangeToInclusive<usize> {}
#[stable(feature = "slice_index_bound_pair", since = "1.51.0")]
impl Sealed for (ops::Bound<usize>, ops::Bound<usize>) {}
}

/// A helper trait used for indexing operations.
Expand Down Expand Up @@ -449,3 +451,104 @@ unsafe impl<T> SliceIndex<[T]> for ops::RangeToInclusive<usize> {
(0..=self.end).index_mut(slice)
}
}

#[stable(feature = "slice_index_bound_pair", since = "1.51.0")]
unsafe impl<T> SliceIndex<[T]> for (ops::Bound<usize>, ops::Bound<usize>) {
type Output = [T];

#[inline]
fn get(self, slice: &[T]) -> Option<&[T]> {
let start = match self.0 {
ops::Bound::Unbounded => 0,
ops::Bound::Included(start) => start,
ops::Bound::Excluded(start) => start.checked_add(1)?,
};
let end = match self.1 {
ops::Bound::Unbounded => slice.len(),
ops::Bound::Included(end) => end.checked_add(1)?,
ops::Bound::Excluded(end) => end,
};
(start..end).get(slice)
}

#[inline]
fn get_mut(self, slice: &mut [T]) -> Option<&mut [T]> {
let start = match self.0 {
ops::Bound::Unbounded => 0,
ops::Bound::Included(start) => start,
ops::Bound::Excluded(start) => start.checked_add(1)?,
};
let end = match self.1 {
ops::Bound::Unbounded => slice.len(),
ops::Bound::Included(end) => end.checked_add(1)?,
ops::Bound::Excluded(end) => end,
};
(start..end).get_mut(slice)
}

#[inline]
unsafe fn get_unchecked(self, slice: *const [T]) -> *const [T] {
let start = match self.0 {
ops::Bound::Unbounded => 0,
ops::Bound::Included(start) => start,
ops::Bound::Excluded(start) => start + 1,
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if it is a good idea to implicitly overflow in get_unchecked (and get_mutunchecked`). The "unchecked" refers to the slice bounds, how is the caller supposed to know that they also need to guard against overflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I used the existing code in RangeInclusive for precedent as that also doesn't guard against overflow. I'm fine just doing a saturated_add instead though.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, that's a fair point. But I still feel like this choice (either way) should be documented somewhere.

};
let end = match self.1 {
ops::Bound::Unbounded => slice.len(),
ops::Bound::Included(end) => end + 1,
ops::Bound::Excluded(end) => end,
};
// SAFETY: the caller has to uphold the safety contract for `get_unchecked`.
unsafe { (start..end).get_unchecked(slice) }
}

#[inline]
unsafe fn get_unchecked_mut(self, slice: *mut [T]) -> *mut [T] {
let start = match self.0 {
ops::Bound::Unbounded => 0,
ops::Bound::Included(start) => start,
ops::Bound::Excluded(start) => start + 1,
};
let end = match self.1 {
ops::Bound::Unbounded => slice.len(),
ops::Bound::Included(end) => end + 1,
ops::Bound::Excluded(end) => end,
};
// SAFETY: the caller has to uphold the safety contract for `get_unchecked_mut`.
unsafe { (start..end).get_unchecked_mut(slice) }
}

#[inline]
fn index(self, slice: &[T]) -> &[T] {
let start = match self.0 {
ops::Bound::Unbounded => 0,
ops::Bound::Included(start) => start,
ops::Bound::Excluded(start) if start == usize::MAX => slice_start_index_overflow_fail(),
ops::Bound::Excluded(start) => start + 1,
};
let end = match self.1 {
ops::Bound::Unbounded => slice.len(),
ops::Bound::Included(end) if end == usize::MAX => slice_end_index_overflow_fail(),
ops::Bound::Included(end) => end + 1,
ops::Bound::Excluded(end) => end,
};
(start..end).index(slice)
Comment on lines +523 to +535
Copy link
Contributor

@dylni dylni Jan 17, 2021

Choose a reason for hiding this comment

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

assert_len can make this simpler:

Suggested change
let start = match self.0 {
ops::Bound::Unbounded => 0,
ops::Bound::Included(start) => start,
ops::Bound::Excluded(start) if start == usize::MAX => slice_start_index_overflow_fail(),
ops::Bound::Excluded(start) => start + 1,
};
let end = match self.1 {
ops::Bound::Unbounded => slice.len(),
ops::Bound::Included(end) if end == usize::MAX => slice_end_index_overflow_fail(),
ops::Bound::Included(end) => end + 1,
ops::Bound::Excluded(end) => end,
};
(start..end).index(slice)
unsafe {
slice.get_unchecked(self.assert_len(slice.len()))
}

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 tried looking for a method like this and apparently couldn't find one, huh.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't worry. I only know about it because I implemented it.

}

#[inline]
fn index_mut(self, slice: &mut [T]) -> &mut [T] {
let start = match self.0 {
ops::Bound::Unbounded => 0,
ops::Bound::Included(start) => start,
ops::Bound::Excluded(start) if start == usize::MAX => slice_start_index_overflow_fail(),
ops::Bound::Excluded(start) => start + 1,
};
let end = match self.1 {
ops::Bound::Unbounded => slice.len(),
ops::Bound::Included(end) if end == usize::MAX => slice_end_index_overflow_fail(),
ops::Bound::Included(end) => end + 1,
ops::Bound::Excluded(end) => end,
};
(start..end).index_mut(slice)
Comment on lines +540 to +552
Copy link
Contributor

Choose a reason for hiding this comment

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

It can be used here too:

Suggested change
let start = match self.0 {
ops::Bound::Unbounded => 0,
ops::Bound::Included(start) => start,
ops::Bound::Excluded(start) if start == usize::MAX => slice_start_index_overflow_fail(),
ops::Bound::Excluded(start) => start + 1,
};
let end = match self.1 {
ops::Bound::Unbounded => slice.len(),
ops::Bound::Included(end) if end == usize::MAX => slice_end_index_overflow_fail(),
ops::Bound::Included(end) => end + 1,
ops::Bound::Excluded(end) => end,
};
(start..end).index_mut(slice)
unsafe {
slice.get_unchecked_mut(self.assert_len(slice.len()))
}

}
}