Skip to content

Add support for slicing with inclusive ranges #467

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

Merged
merged 2 commits into from
Aug 28, 2018

Conversation

jturner314
Copy link
Member

Rust 1.26 added support for inclusive ranges, which can be created with syntax start..=end or ..=end.

The behavior of this implementation is a little weird in the rare case that range.end == ::std::isize::MAX due to overflow, but without changing the representation of Slice and SliceOrIndex, I don't see a good way to handle that case cleanly. This shouldn't be a significant issue in practice because:

  1. It's rare to have range.end == ::std::isize::MAX.
  2. The overflow will result in a panic in debug mode, which is reasonable behavior.
  3. In release mode, the overflow will result in slice.end = 0, which will result in a panic when slicing unless slice.start == 0, which will result in an empty slice. The panic is reasonable behavior. The empty slice behavior in the slice.start == 0 case isn't ideal, but I wouldn't expect it to result in any significant problems.

Rust 1.26 added support for inclusive ranges, which can be created
with syntax `start..=end` or `..=end`. This commit bumps the required
Rust version to 1.27 because the `.start()` and `.end()` methods of
`RangeInclusive` were added only in 1.27.
@jturner314 jturner314 force-pushed the slice-inclusive-ranges branch from f90f843 to 4405d36 Compare June 24, 2018 22:21
@jturner314
Copy link
Member Author

I just pushed a new version because it turns out that the .start() and .end() methods of RangeInclusive were added only in Rust 1.27, so this PR requires a bump in ndarray's minimum Rust version to 1.27.

Copy link
Collaborator

@nilgoyette nilgoyette left a comment

Choose a reason for hiding this comment

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

Some questions about macros, but apart from that, lgtm.

src/slice.rs Outdated
@@ -453,6 +502,12 @@ impl<D1: Dimension, T> SliceNextDim<D1, D1::Larger> for Range<T> {
}
}

impl<D1: Dimension, T> SliceNextDim<D1, D1::Larger> for RangeInclusive<T> {
fn next_dim(&self, _: PhantomData<D1>) -> PhantomData<D1::Larger> {
PhantomData
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another macro question :) From line 499 to 527, would it be possible to create a macro for that? next_dim is implemented for all types of Range except RangeFull.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is a lot more concise with a macro. Thanks!

step: 1,
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Line 70 to 129 and 265 to 324. do you think it would a good place for a macro? They are already macros, but it could be a merge of impl_slice_from_index_type and impl_sliceorindex_from_index_type. The only change in all those lines is Slice and SliceOrIndex.

It's a real question btw. Maybe too much macro make the code unreadable or un-fun or wathever. Or maybe the special case From<$index> make this more complex?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've merged the macros as you've suggested, which substantially reduced the duplication. The special case is not an issue; I've kept a separate macro for it.

Thanks for pointing this out!

@bluss
Copy link
Member

bluss commented Jul 13, 2018

My usual attention for inclusive ranges would be to overflow issues, and I haven't really looked closely enough at that here

@jturner314
Copy link
Member Author

A few notes:

  1. I was wrong about the overflow; the result of ::std::isize::MAX + 1 in release mode is ::std::isize::MIN, not zero.

  2. An inclusive range with an end of ::std::isize::MAX for slicing is always a user error, so the behavior in this PR is reasonable. (The maximum length of an allocation is Rust is ::std::isize::MAX, so an index of ::std::isize::MAX is one past the maximum possible index ::std::isize::MAX - 1.)

  3. I double-checked dimension::do_slice to make sure that even with a weird index value due to overflow, it's not possible to cause unsafe behavior.

@jturner314 jturner314 merged commit a1621f3 into rust-ndarray:master Aug 28, 2018
@jturner314 jturner314 deleted the slice-inclusive-ranges branch August 28, 2018 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants