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

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Jan 17, 2021

This effectively allows using the RangeBounds trait to index slices, since there aren't any existing ranges that would work otherwise with a Bound::Excluded start. This basically uses the same strategy as RangeInclusive and just fails if you're trying to do a range starting at exclusive usize::MAX, since that effectively would never work.

Would require FCP but considering how pairs of bounds are allowed for RangeBounds, it seems reasonable to allow them for slices too.

(Working on adding tests.)

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 17, 2021
@clarfonthey clarfonthey force-pushed the index_bounds branch 6 times, most recently from 16bd67d to 97c0c24 Compare January 17, 2021 05:34
Comment on lines +523 to +535
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)
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.

Comment on lines +540 to +552
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)
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()))
}

@dylni dylni mentioned this pull request Jan 17, 2021
4 tasks
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.

@RalfJung
Copy link
Member

This effectively allows using the RangeBounds trait to index slices, since there aren't any existing ranges that would work otherwise with a Bound::Excluded start.

So, could you give some examples for how you imagine this could be used? The PR has no testcase at all so this is a bit hard to see.

@dylni
Copy link
Contributor

dylni commented Jan 18, 2021

I just found this PR, which should add the same impls. It has an active FCP, and it has the same question of what its use case would be.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 24, 2021

I just found this PR, which should add the same impls. It has an active FCP, and it has the same question of what its use case would be.

Closing this as a duplicate of #77704. Feel free to join the discussion there. Thanks!

@m-ou-se m-ou-se closed this Jan 24, 2021
@m-ou-se m-ou-se added A-slice Area: `[T]` T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 24, 2021
@clarfonthey clarfonthey deleted the index_bounds branch March 14, 2021 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-slice Area: `[T]` S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

5 participants