Add multislice! macro #388
Conversation
jturner314
force-pushed
the
multi-slice
branch
from
77336f3 to
97d5c20
Compare
jturner314
force-pushed
the
multi-slice
branch
2 times, most recently
from
c136b72 to
8b862be
Compare
jturner314
force-pushed
the
multi-slice
branch
from
8b862be to
4456819
Compare
|
Very well written PR! |
|
Do you want me to look into the Raw pointer array view? |
That would be great, thanks. I have a start on the implementation in jturner314/rust-ndarray/dataraw-trait but I don't have much time to work on it at the moment. |
jturner314
force-pushed
the
multi-slice
branch
from
e7899a3 to
38460e0
Compare
|
I've updated the PR to use the Now (using let (a, b, c, d) = multislice!(arr, (mut s_a, s_b, mut s_c, s_d));expands to the following (generated with let (a /* 107#0 */, b /* 108#0 */, c /* 109#0 */, d /* 110#0 */) =
{
let (life /* 11149#22 */, raw_view /* 11021#22 */) =
{
let mut view /* 10978#22 */ =
::ArrayBase /* 10959#22 */::view_mut /* 10979#22
*/(&mut arr /* 93#0 */);
(::life_of_view_mut /* 10783#22 */(&view /* 10978#22 */),
view /* 10978#22 */.raw_view_mut /* 11022#22 */())
};
match s_a /* 100#0 */ {
info /* 11259#23 */ => {
match s_b /* 103#0 */ {
info /* 11259#24 */ => {
assert!(!::slices_intersect /* 10954#25
*/(&raw_view /* 11021#22 */.raw_dim /*
10799#25 */(),
info /* 11259#24 */,
info /* 11259#23 */));
match s_c /* 104#0 */ {
info /* 11259#36 */ => {
assert!(!::slices_intersect /* 10954#37
*/(&raw_view /* 11021#22
*/.raw_dim /* 10799#37
*/(),
info /* 11259#36 */,
info /* 11259#23 */));
assert!(!::slices_intersect /* 10954#48
*/(&raw_view /* 11021#22
*/.raw_dim /* 10799#48
*/(),
info /* 11259#36 */,
info /* 11259#24 */));
match s_d /* 106#0 */ {
info /* 11259#60 */ => {
{
assert!(!::slices_intersect /*
10954#62
*/(&raw_view /*
11021#22
*/.raw_dim /*
10799#62
*/(),
info /* 11259#60
*/,
info /* 11259#23
*/));
assert!(!::slices_intersect /*
10954#73
*/(&raw_view /*
11021#22
*/.raw_dim /*
10799#73
*/(),
info /* 11259#60
*/,
info /* 11259#36
*/));
};
(#[allow(unsafe_code)] unsafe {
::deref_raw_view_mut_into_view_mut_with_life
/*
10785#84
*/(raw_view
/*
11021#22
*/.clone
/*
474#84
*/().slice_move
/*
10983#84
*/(info
/*
11259#23
*/),
life
/*
11149#22
*/)
},
#[allow(unsafe_code)] unsafe {
::deref_raw_view_mut_into_view_with_life
/*
10784#85
*/(raw_view
/*
11021#22
*/.clone
/*
474#85
*/().slice_move
/*
10983#85
*/(info
/*
11259#24
*/),
life
/*
11149#22
*/)
},
#[allow(unsafe_code)] unsafe {
::deref_raw_view_mut_into_view_mut_with_life
/*
10785#86
*/(raw_view
/*
11021#22
*/.clone
/*
474#86
*/().slice_move
/*
10983#86
*/(info
/*
11259#36
*/),
life
/*
11149#22
*/)
},
#[allow(unsafe_code)] unsafe {
::deref_raw_view_mut_into_view_with_life
/*
10784#87
*/(raw_view
/*
11021#22
*/.clone
/*
474#87
*/().slice_move
/*
10983#87
*/(info
/*
11259#60
*/),
life
/*
11149#22
*/)
})
}
}
}
}
}
}
}
}
};I'd like to add a few more tests, and I'm strongly considering returning an error instead of panicking when the slices violate the aliasing rules, but otherwise, this PR is mostly done. Any thoughts on the error vs. panic question? |
src/slice.rs
Outdated
| /// disjoint). | ||
| /// | ||
| /// The syntax is `multislice!(` *expression, (pattern [, pattern [, …]])* `)`, | ||
| /// where *expression* evaluates to an `ArrayBase<S, D>` where `S: DataMut`, |
There was a problem hiding this comment.
I guess you disagree (since you wrote it this way), but I'd prefer to say "mutable array" rather than "ArrayBase<S, D> where S: DataMut". (Yes I'm constantly unhappy with exposing ArrayBase, DataMut like this 🙂 .)
There was a problem hiding this comment.
I like "mutable array" better too. I think I originally wrote it that way because macros don't have type signatures like methods do, which tends to make me want to be fairly explicit about the types they expect and return. In this case, though, "mutable array" is clear and less verbose.
|
I'll have to get back to reviewing this (hopefully soon), but amazing work! I would first think that indexing errors should, “as usual”, result in a panic. If it's an error -- what would the user do with it? |
| /// let j = arr2(&[[1, 3], | ||
| /// [5, 7]]); | ||
| /// assert_eq!(s0, i); | ||
| /// assert_eq!(s1, j); |
There was a problem hiding this comment.
🔥 🔥 🔥 🔥
(Just the feature itself is really nice.)
src/slice.rs
Outdated
| /// | ||
| /// # fn main() { | ||
| /// let mut arr = Array1::from_iter(0..12); | ||
| /// let (a, b, c, d) = multislice!(arr, (s![0..5], mut s![6..;2], s![1..6], mut s![7..;2])); |
There was a problem hiding this comment.
Nested macros -- s![] inside multislice seem natural enough. But we and others will ask.. is that extra s! thing necessary, this is already a macro?
But of course, there is not much that can be simplified, we need some kind of border to group each slice, and the s! syntax is at least already familiar.
There was a problem hiding this comment.
That's a good point. I wrote it that way because it was the most obvious way to allow the user to supply a &SliceInfo instance instead of always requiring them to use s!-style notation. However, it would be possible to use pattern-matching to differentiate between s!-style notation and an expression that evaluates to &SliceInfo, so the s! isn't strictly necessary. That said, I think that we should keep it as-is for the following reasons:
- It's easier to read
multislice!(arr, (s![0..5], mut s![6..;2]))at a glance thanmultislice!(arr, ([0..5], mut [6..;2]))if you have syntax highlighting because thes!is shown in a different color, which makes it really easy to see the grouping of the slicing info. - When the user writes
multislice!(arr, (s![0..5], mut s![6..;2])), it's clear what thes!means because thes!macro is used fairly often. Without thes!, it's not quite as obvious. - The
multislice!implementation is a little simpler without having to provide special parsing fors!-style expressions.
There was a problem hiding this comment.
I'm curious, is it technically possible to use this syntax: multislice!(arr, [0..5], mut [6..;2])?
There was a problem hiding this comment.
Yes; we can just add rules that apply the s![] macro, matching on [$($info:tt)*], above the rules matching on $info:expr:
diff --git a/src/slice.rs b/src/slice.rs
index 0d358f1..a06a8fd 100644
--- a/src/slice.rs
+++ b/src/slice.rs
@@ -770,6 +770,108 @@ macro_rules! multislice(
)
}
};
+ // Parse last slice (mutable), no trailing comma, applying `s![]` macro.
+ (
+ @parse $view:expr, $life:expr,
+ ($($sliced:tt)*),
+ ($($mut_info:tt)*),
+ ($($immut_info:tt)*),
+ (mut [$($info:tt)*])
+ ) => {
+ // Apply `s![]` macro to info.
+ $crate::multislice!(
+ @parse $view, $life,
+ ($($sliced)*),
+ ($($mut_info)*),
+ ($($immut_info)*),
+ (mut s![$($info)*],)
+ )
+ };
+ // Parse last slice (read-only), no trailing comma, applying `s![]` macro.
+ (
+ @parse $view:expr, $life:expr,
+ ($($sliced:tt)*),
+ ($($mut_info:tt)*),
+ ($($immut_info:tt)*),
+ ([$($info:tt)*])
+ ) => {
+ // Apply `s![]` macro to info.
+ $crate::multislice!(
+ @parse $view, $life,
+ ($($sliced)*),
+ ($($mut_info)*),
+ ($($immut_info)*),
+ (s![$($info)*],)
+ )
+ };
+ // Parse last slice (mutable), with trailing comma, applying `s![]` macro.
+ (
+ @parse $view:expr, $life:expr,
+ ($($sliced:tt)*),
+ ($($mut_info:tt)*),
+ ($($immut_info:tt)*),
+ (mut [$($info:tt)*],)
+ ) => {
+ // Apply `s![]` macro to info.
+ $crate::multislice!(
+ @parse $view, $life,
+ ($($sliced)*),
+ ($($mut_info)*),
+ ($($immut_info)*),
+ (mut s![$($info)*],)
+ )
+ };
+ // Parse last slice (read-only), with trailing comma, applying `s![]` macro.
+ (
+ @parse $view:expr, $life:expr,
+ ($($sliced:tt)*),
+ ($($mut_info:tt)*),
+ ($($immut_info:tt)*),
+ ([$($info:tt)*],)
+ ) => {
+ // Apply `s![]` macro to info.
+ $crate::multislice!(
+ @parse $view, $life,
+ ($($sliced)*),
+ ($($mut_info)*),
+ ($($immut_info)*),
+ (s![$($info)*],)
+ )
+ };
+ // Parse a mutable slice, applying `s![]` macro.
+ (
+ @parse $view:expr, $life:expr,
+ ($($sliced:tt)*),
+ ($($mut_info:tt)*),
+ ($($immut_info:tt)*),
+ (mut [$($info:tt)*], $($t:tt)*)
+ ) => {
+ // Apply `s![]` macro to info.
+ $crate::multislice!(
+ @parse $view, $life,
+ ($($sliced)*),
+ ($($mut_info)*),
+ ($($immut_info)*),
+ (mut s![$($info)*], $($t)*)
+ )
+ };
+ // Parse a read-only slice, applying `s![]` macro.
+ (
+ @parse $view:expr, $life:expr,
+ ($($sliced:tt)*),
+ ($($mut_info:tt)*),
+ ($($immut_info:tt)*),
+ ([$($info:tt)*], $($t:tt)*)
+ ) => {
+ // Apply `s![]` macro to info.
+ $crate::multislice!(
+ @parse $view, $life,
+ ($($sliced)*),
+ ($($mut_info)*),
+ ($($immut_info)*),
+ (s![$($info)*], $($t)*)
+ )
+ };
// Parse last slice (mutable), no trailing comma.
(
@parse $view:expr, $life:expr,This change would be backwards compatible if we want to add it in the future.
There was a problem hiding this comment.
Just counting in syntactic noise, it seems like a big win to drop both the extra () around the slices and each s!, I would go for that right away. The s! would keep working, because you've designed it so we can insert arbitrary expressions for each axis output, right? You could do (?)
let ax1 = s![1..];
let ax2 = s![2..];
multislice!(arr, ax1, ax2);I guess it would count as us breaking array expressions (like [1,2,3]) in those locations, but there is no reason to use those there at this point.
There was a problem hiding this comment.
Oh, I didn't notice the dropped () in your earlier comment. Yeah, we can drop the () too. With the extra () dropped, I think it's fine to drop the s! too. (With the (), it looks like a tuple, which makes me want to require the extra s! so that the elements are expressions. Without the (), the parameters are clearly parameters to the macro and not part of a tuple, so IMO it's fine to do things like interpreting them as s!-style syntax without requiring the s!.)
I'll update the PR to drop the extra () and s!.
The
s!would keep working, because you've designed it so we can insert arbitrary expressions for each output, right?
Yes, normal expressions that evaluate to &SliceInfo will still work. (They just have a lower precedence than [$($info:tt)*].)
I guess it would count as us breaking array expressions (like
[1,2,3]) in those locations, but there is no reason to use those there at this point.
Yeah, these don't matter right now, because the slicing expressions must have type &SliceInfo, and an expression of the form [$($info:tt)*] is always a fixed-length array, not a &SliceInfo. If we want to add support for slicing using fixed-length array expressions in the future (e.g. [1, 2, 3] and [0..5, 3..7, 2..8] for Ix3) that don't use the s! macro machinery, we can do so by adding more rules with even higher precedence. (They'd match on e.g. [$($info:expr),*].)
One interesting thing to note is that we could implement multislice! as a method on ArrayBase, instead of a macro, that could be called like this:
arr.multislice((s![0..5], Mut(s![6..;2])))
// or
arr.multislice(ss!([0..5], mut [6..;2]))but that would work up to only a limited tuple length (however long we'd decide to support), like Zip works up to only 6 producers.
There was a problem hiding this comment.
As a method sounds cool but the syntax doesn't work quite so well there, so it doesn't seem as good. Hopefully multislice as a macro has its own attraction by virtue of its functionality so that users find it.
There was a problem hiding this comment.
I actually think arr.multislice(ss!([0..5], mut [6..;2])) is quite nice, but it would be more complex to implement than the multislice! macro.
I've updated the PR to drop the extra () and s!.
Out-of-bounds indices should panic, but there isn't precedent for how to handle overlapping slices, so we could go either way. The primary issue with panicking is that determining whether slices will overlap for arbitrary indices is somewhat tricky, and I like to keep panic conditions simple and easy-to-avoid. Additionally, if we return an error, the user can easily convert it into a panic with
That's a really good question. If there is an overlap, is there a feasible way to recover? I have trouble coming up with a realistic case where you'd want to do anything other than panic. I've been using So, on further thought, I think that |
|
This is ready to go, right? Everything looks good to me! Then we can soon release 0.13 with this and updated matrixmultiply |
|
I looked over the PR again and merged it. I'm glad to get this merged; I'll finally be able to use mainline
I have a couple more breaking changes I'd like to make in order to support more slicing operations. I'll submit the PR today. |
This PR adds a
multislice!()macro which makes it possible to take multiple simultaneous mutable (and read-only) slices of an array. It panics at runtime if slices violate Rust's aliasing rules.Regarding the implementation:
It's a lot of new lines of code, but most of the new lines are comments, docs, and tests.
Most of the new functions are in
dimension/mod.rs. They could also be put inslice.rs, whatever makes more sense.Additions to the public API are hidden from the docs (except for
multislice!()).Basically, the way
multislice!()works is to borrow the array a single time by callingview_mut(), and then copy the borrow as many times as needed to create the resultingArrayViewandArrayViewMutinstances. This ensures that the underlying array is mutably borrowed and all the lifetimes are correct.I didn't see a good way to do that with the existing public API, so I added two (hidden) methods to
ArrayViewMut:aliasing_view()andaliasing_view_mut().The primary disadvantage of this approach is that it always mutably borrows the array, even if there aren't any
mutslices, but that's not a big issue because if you aren't taking anymutslices, you can just call.slice()multiple times anyway.This is the first time I've used
quickcheck, and I love it! I caught a few bugs in my implementation on edge cases withquickchecktests.Please let me know what you think!
Edit: Would it be better to return an
Errinstead of panicking if the slices violate the aliasing rules?