From 8a51ff925c770650d5a3f59729c8159e35ccfdbd Mon Sep 17 00:00:00 2001 From: Jim Turner Date: Tue, 20 Aug 2019 19:54:20 -0400 Subject: [PATCH] Avoid re-borrowing in multislice! macro The old implementation always called `.view_mut()` on the input array, which meant that it was not possible to use `multislice!` to split an `ArrayViewMut` instance into pieces with the same lifetime as the original view. Now, `multislice!` uses `ArrayViewMut::from()` instead so that this works. The primary disadvantage is that an explicit borrow is necessary in many cases, which is less concise. Closes #687. --- src/lib.rs | 2 +- src/slice.rs | 18 +++++----- tests/array.rs | 98 ++++++++++++++++++++++++++++++-------------------- 3 files changed, 69 insertions(+), 49 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index e5af0863c..2f5631c69 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -524,7 +524,7 @@ pub type Ixs = isize; /// // - One containing all the odd-index columns in the matrix /// let mut h = arr2(&[[0, 1, 2, 3], /// [4, 5, 6, 7]]); -/// let (s0, s1) = multislice!(h, mut [.., ..;2], mut [.., 1..;2]); +/// let (s0, s1) = multislice!(&mut h, mut [.., ..;2], mut [.., 1..;2]); /// let i = arr2(&[[0, 2], /// [4, 6]]); /// let j = arr2(&[[1, 3], diff --git a/src/slice.rs b/src/slice.rs index 85abfb8e0..1b73e3f54 100644 --- a/src/slice.rs +++ b/src/slice.rs @@ -657,8 +657,8 @@ pub unsafe fn deref_raw_view_mut_into_view_mut_with_life<'a, A, D: Dimension>( /// disjoint). /// /// The syntax is `multislice!(` *expression, pattern [, pattern [, …]]* `)`, -/// where *expression* evaluates to a mutable array, and each *pattern* is -/// either +/// where *expression* is any valid input to `ArrayViewMut::from()`, and each +/// *pattern* is either /// /// * `mut` *s-args-or-expr*: creates an `ArrayViewMut` or /// * *s-args-or-expr*: creates an `ArrayView` @@ -667,9 +667,9 @@ pub unsafe fn deref_raw_view_mut_into_view_mut_with_life<'a, A, D: Dimension>( /// the [`s!`] macro to create a `&SliceInfo` instance or (2) an expression /// that evaluates to a `&SliceInfo` instance. /// -/// **Note** that this macro always mutably borrows the array even if there are -/// no `mut` patterns. If all you want to do is take read-only slices, you -/// don't need `multislice!()`; just call +/// **Note** that *expression* is converted to an `ArrayViewMut` using +/// `ArrayViewMut::from()` even if there are no `mut` patterns. If all you want +/// to do is take read-only slices, you don't need `multislice!()`; just call /// [`.slice()`](struct.ArrayBase.html#method.slice) multiple times instead. /// /// `multislice!()` evaluates to a tuple of `ArrayView` and/or `ArrayViewMut` @@ -697,7 +697,7 @@ pub unsafe fn deref_raw_view_mut_into_view_mut_with_life<'a, A, D: Dimension>( /// /// # fn main() { /// let mut arr: Array1<_> = (0..12).collect(); -/// let (a, b, c, d) = multislice!(arr, [0..5], mut [6..;2], [1..6], mut [7..;2]); +/// let (a, b, c, d) = multislice!(&mut arr, [0..5], mut [6..;2], [1..6], mut [7..;2]); /// assert_eq!(a, array![0, 1, 2, 3, 4]); /// assert_eq!(b, array![6, 8, 10]); /// assert_eq!(c, array![1, 2, 3, 4, 5]); @@ -715,7 +715,7 @@ pub unsafe fn deref_raw_view_mut_into_view_mut_with_life<'a, A, D: Dimension>( /// # use ndarray::prelude::*; /// # fn main() { /// let mut arr: Array1<_> = (0..12).collect(); -/// multislice!(arr, [0..5], mut [1..;2]); // panic! +/// multislice!(&mut arr, [0..5], mut [1..;2]); // panic! /// # } /// ``` /// @@ -727,7 +727,7 @@ pub unsafe fn deref_raw_view_mut_into_view_mut_with_life<'a, A, D: Dimension>( /// # use ndarray::prelude::*; /// # fn main() { /// let mut arr: Array1<_> = (0..12).collect(); -/// multislice!(arr, mut [0..5], mut [1..;2]); // panic! +/// multislice!(&mut arr, mut [0..5], mut [1..;2]); // panic! /// # } /// ``` #[macro_export] @@ -985,7 +985,7 @@ macro_rules! multislice( ($arr:expr, $($t:tt)*) => { { let (life, raw_view) = { - let mut view = $crate::ArrayBase::view_mut(&mut $arr); + let mut view: $crate::ArrayViewMut<_, _> = ::core::convert::From::from($arr); ($crate::life_of_view_mut(&view), view.raw_view_mut()) }; $crate::multislice!(@parse raw_view, life, (), (), (), ($($t)*)) diff --git a/tests/array.rs b/tests/array.rs index 70802e2c5..61227420c 100644 --- a/tests/array.rs +++ b/tests/array.rs @@ -336,76 +336,74 @@ fn test_slice_collapse_with_indices() { } #[test] -#[allow(clippy::cognitive_complexity)] fn test_multislice() { - defmac!(test_multislice mut arr, s1, s2 => { - { - let copy = arr.clone(); + macro_rules! check_multislice { + ($arr:expr, $s1:expr, $s2:expr) => {{ + let copy = $arr.clone(); assert_eq!( - multislice!(arr, mut s1, mut s2,), - (copy.clone().slice_mut(s1), copy.clone().slice_mut(s2)) + multislice!(&mut $arr, mut $s1, mut $s2,), + (copy.clone().slice_mut($s1), copy.clone().slice_mut($s2)) ); } { - let copy = arr.clone(); + let copy = $arr.clone(); assert_eq!( - multislice!(arr, mut s1, s2,), - (copy.clone().slice_mut(s1), copy.clone().slice(s2)) + multislice!(&mut $arr, mut $s1, $s2,), + (copy.clone().slice_mut($s1), copy.clone().slice($s2)) ); } { - let copy = arr.clone(); + let copy = $arr.clone(); assert_eq!( - multislice!(arr, s1, mut s2), - (copy.clone().slice(s1), copy.clone().slice_mut(s2)) + multislice!(&mut $arr, $s1, mut $s2), + (copy.clone().slice($s1), copy.clone().slice_mut($s2)) ); } { - let copy = arr.clone(); + let copy = $arr.clone(); assert_eq!( - multislice!(arr, s1, s2), - (copy.clone().slice(s1), copy.clone().slice(s2)) + multislice!(&mut $arr, $s1, $s2), + (copy.clone().slice($s1), copy.clone().slice($s2)) ); - } - }); + }}; + }; let mut arr = Array1::from_iter(0..48).into_shape((8, 6)).unwrap(); - - assert_eq!((arr.clone().view(),), multislice!(arr, [.., ..])); - test_multislice!(&mut arr, s![0, ..], s![1, ..]); - test_multislice!(&mut arr, s![0, ..], s![-1, ..]); - test_multislice!(&mut arr, s![0, ..], s![1.., ..]); - test_multislice!(&mut arr, s![1, ..], s![..;2, ..]); - test_multislice!(&mut arr, s![..2, ..], s![2.., ..]); - test_multislice!(&mut arr, s![1..;2, ..], s![..;2, ..]); - test_multislice!(&mut arr, s![..;-2, ..], s![..;2, ..]); - test_multislice!(&mut arr, s![..;12, ..], s![3..;3, ..]); + assert_eq!((arr.clone().view(),), multislice!(&mut arr, [.., ..])); + check_multislice!(arr, s![0, ..], s![1, ..]); + check_multislice!(arr, s![0, ..], s![-1, ..]); + check_multislice!(arr, s![0, ..], s![1.., ..]); + check_multislice!(arr, s![1, ..], s![..;2, ..]); + check_multislice!(arr, s![..2, ..], s![2.., ..]); + check_multislice!(arr, s![1..;2, ..], s![..;2, ..]); + check_multislice!(arr, s![..;-2, ..], s![..;2, ..]); + check_multislice!(arr, s![..;12, ..], s![3..;3, ..]); } #[test] fn test_multislice_intersecting() { assert_panics!({ let mut arr = Array2::::zeros((8, 6)); - multislice!(arr, mut [3, ..], [3, ..]); + multislice!(&mut arr, mut [3, ..], [3, ..]); }); assert_panics!({ let mut arr = Array2::::zeros((8, 6)); - multislice!(arr, mut [3, ..], [3.., ..]); + multislice!(&mut arr, mut [3, ..], [3.., ..]); }); assert_panics!({ let mut arr = Array2::::zeros((8, 6)); - multislice!(arr, mut [3, ..], [..;3, ..]); + multislice!(&mut arr, mut [3, ..], [..;3, ..]); }); assert_panics!({ let mut arr = Array2::::zeros((8, 6)); - multislice!(arr, mut [..;6, ..], [3..;3, ..]); + multislice!(&mut arr, mut [..;6, ..], [3..;3, ..]); }); assert_panics!({ let mut arr = Array2::::zeros((8, 6)); - multislice!(arr, mut [2, ..], mut [..-1;-2, ..]); + multislice!(&mut arr, mut [2, ..], mut [..-1;-2, ..]); }); { let mut arr = Array2::::zeros((8, 6)); - multislice!(arr, [3, ..], [-1..;-2, ..]); + multislice!(&mut arr, [3, ..], [-1..;-2, ..]); } } @@ -418,7 +416,7 @@ fn test_multislice_eval_args_only_once() { eval_count += 1; *s![1..2] }; - multislice!(arr, mut &slice(), [3..4], [5..6]); + multislice!(&mut arr, mut &slice(), [3..4], [5..6]); } assert_eq!(eval_count, 1); let mut eval_count = 0; @@ -427,7 +425,7 @@ fn test_multislice_eval_args_only_once() { eval_count += 1; *s![1..2] }; - multislice!(arr, [3..4], mut &slice(), [5..6]); + multislice!(&mut arr, [3..4], mut &slice(), [5..6]); } assert_eq!(eval_count, 1); let mut eval_count = 0; @@ -436,7 +434,7 @@ fn test_multislice_eval_args_only_once() { eval_count += 1; *s![1..2] }; - multislice!(arr, [3..4], [5..6], mut &slice()); + multislice!(&mut arr, [3..4], [5..6], mut &slice()); } assert_eq!(eval_count, 1); let mut eval_count = 0; @@ -445,7 +443,7 @@ fn test_multislice_eval_args_only_once() { eval_count += 1; *s![1..2] }; - multislice!(arr, &slice(), mut [3..4], [5..6]); + multislice!(&mut arr, &slice(), mut [3..4], [5..6]); } assert_eq!(eval_count, 1); let mut eval_count = 0; @@ -454,7 +452,7 @@ fn test_multislice_eval_args_only_once() { eval_count += 1; *s![1..2] }; - multislice!(arr, mut [3..4], &slice(), [5..6]); + multislice!(&mut arr, mut [3..4], &slice(), [5..6]); } assert_eq!(eval_count, 1); let mut eval_count = 0; @@ -463,11 +461,33 @@ fn test_multislice_eval_args_only_once() { eval_count += 1; *s![1..2] }; - multislice!(arr, mut [3..4], [5..6], &slice()); + multislice!(&mut arr, mut [3..4], [5..6], &slice()); } assert_eq!(eval_count, 1); } +#[test] +fn test_multislice_arrayviewmut_same_life() { + // This test makes sure that it's possible for the borrowed elements + // returned from `get_mut2` to have the same life as the `arr` view. + fn get_mut2<'a, A>( + arr: ArrayViewMut<'a, A, Ix2>, + [i1, j1]: [usize; 2], + [i2, j2]: [usize; 2], + ) -> (&'a mut A, &'a mut A) { + use ndarray::IndexLonger; + let (x1, x2) = multislice!(arr, mut [i1, j1], mut [i2, j2]); + (x1.index([]), x2.index([])) + } + let mut arr = array![[1, 2], [3, 4]]; + { + let (x1, x2) = get_mut2(arr.view_mut(), [0, 0], [1, 0]); + *x1 += 1; + *x2 += 2; + } + assert_eq!(arr, array![[2, 2], [5, 4]]); +} + #[should_panic] #[test] fn index_out_of_bounds() {