Merge pull request #998 from jturner314/fix-offset_from_ptr_to_memory

Fix offset_from_ptr_to_memory for axis lengths of 0, and replace it with offset_from_low_addr_ptr_to_logical_ptr

src/dimension/mod.rs

@@ -235,11 +235,12 @@ where
 /// units of `A` and in units of bytes between the least address and greatest
 /// address accessible by moving along all axes does not exceed `isize::MAX`.
 ///
-/// Warning: This function is sufficient to check the invariants of ArrayBase only
-/// if the pointer to the first element of the array is chosen such that the element
-/// with the smallest memory address is at the start of data. (In other words, the
-/// pointer to the first element of the array must be computed using offset_from_ptr_to_memory
-/// so that negative strides are correctly handled.)
+/// Warning: This function is sufficient to check the invariants of ArrayBase
+/// only if the pointer to the first element of the array is chosen such that
+/// the element with the smallest memory address is at the start of the
+/// allocation. (In other words, the pointer to the first element of the array
+/// must be computed using `offset_from_low_addr_ptr_to_logical_ptr` so that
+/// negative strides are correctly handled.)
 pub(crate) fn can_index_slice<A, D: Dimension>(
     data: &[A],
     dim: &D,
@@ -407,17 +408,19 @@ fn to_abs_slice(axis_len: usize, slice: Slice) -> (usize, usize, isize) {
     (start, end, step)
 }
 
-/// This function computes the offset from the logically first element to the first element in
-/// memory of the array. The result is always <= 0.
-pub fn offset_from_ptr_to_memory<D: Dimension>(dim: &D, strides: &D) -> isize {
-    let offset = izip!(dim.slice(), strides.slice()).fold(0, |_offset, (d, s)| {
-        if (*s as isize) < 0 {
-            _offset + *s as isize * (*d as isize - 1)
+/// Returns the offset from the lowest-address element to the logically first
+/// element.
+pub fn offset_from_low_addr_ptr_to_logical_ptr<D: Dimension>(dim: &D, strides: &D) -> usize {
+    let offset = izip!(dim.slice(), strides.slice()).fold(0, |_offset, (&d, &s)| {
+        let s = s as isize;
+        if s < 0 && d > 1 {
+            _offset - s * (d as isize - 1)
         } else {
             _offset
         }
     });
-    offset
+    debug_assert!(offset >= 0);
+    offset as usize
 }
 
 /// Modify dimension, stride and return data pointer offset

src/impl_constructors.rs

@@ -20,7 +20,7 @@ use alloc::vec;
 use alloc::vec::Vec;
 
 use crate::dimension;
-use crate::dimension::offset_from_ptr_to_memory;
+use crate::dimension::offset_from_low_addr_ptr_to_logical_ptr;
 use crate::error::{self, ShapeError};
 use crate::extension::nonnull::nonnull_from_vec_data;
 use crate::imp_prelude::*;
@@ -492,7 +492,7 @@ where
         // debug check for issues that indicates wrong use of this constructor
         debug_assert!(dimension::can_index_slice(&v, &dim, &strides).is_ok());
 
-        let ptr = nonnull_from_vec_data(&mut v).offset(-offset_from_ptr_to_memory(&dim, &strides));
+        let ptr = nonnull_from_vec_data(&mut v).add(offset_from_low_addr_ptr_to_logical_ptr(&dim, &strides));
         ArrayBase::from_data_ptr(DataOwned::new(v), ptr).with_strides_dim(strides, dim)
     }
 

src/impl_methods.rs

@@ -20,7 +20,7 @@ use crate::dimension;
 use crate::dimension::IntoDimension;
 use crate::dimension::{
     abs_index, axes_of, do_slice, merge_axes, move_min_stride_axis_to_last,
-    offset_from_ptr_to_memory, size_of_shape_checked, stride_offset, Axes,
+    offset_from_low_addr_ptr_to_logical_ptr, size_of_shape_checked, stride_offset, Axes,
 };
 use crate::dimension::broadcast::co_broadcast;
 use crate::dimension::reshape_dim;
@@ -1539,10 +1539,10 @@ where
         S: Data,
     {
         if self.is_contiguous() {
-            let offset = offset_from_ptr_to_memory(&self.dim, &self.strides);
+            let offset = offset_from_low_addr_ptr_to_logical_ptr(&self.dim, &self.strides);
             unsafe {
                 Some(slice::from_raw_parts(
-                    self.ptr.offset(offset).as_ptr(),
+                    self.ptr.sub(offset).as_ptr(),
                     self.len(),
                 ))
             }
@@ -1568,10 +1568,10 @@ where
     {
         if self.is_contiguous() {
             self.ensure_unique();
-            let offset = offset_from_ptr_to_memory(&self.dim, &self.strides);
+            let offset = offset_from_low_addr_ptr_to_logical_ptr(&self.dim, &self.strides);
             unsafe {
                 Ok(slice::from_raw_parts_mut(
-                    self.ptr.offset(offset).as_ptr(),
+                    self.ptr.sub(offset).as_ptr(),
                     self.len(),
                 ))
             }

src/impl_views/constructors.rs

@@ -13,7 +13,7 @@ use crate::error::ShapeError;
 use crate::extension::nonnull::nonnull_debug_checked_from_ptr;
 use crate::imp_prelude::*;
 use crate::{is_aligned, StrideShape};
-use crate::dimension::offset_from_ptr_to_memory;
+use crate::dimension::offset_from_low_addr_ptr_to_logical_ptr;
 
 /// Methods for read-only array views.
 impl<'a, A, D> ArrayView<'a, A, D>
@@ -57,7 +57,7 @@ where
         let dim = shape.dim;
         dimension::can_index_slice_with_strides(xs, &dim, &shape.strides)?;
         let strides = shape.strides.strides_for_dim(&dim);
-        unsafe { Ok(Self::new_(xs.as_ptr().offset(-offset_from_ptr_to_memory(&dim, &strides)), dim, strides)) }
+        unsafe { Ok(Self::new_(xs.as_ptr().add(offset_from_low_addr_ptr_to_logical_ptr(&dim, &strides)), dim, strides)) }
     }
 
     /// Create an `ArrayView<A, D>` from shape information and a raw pointer to
@@ -154,7 +154,7 @@ where
         let dim = shape.dim;
         dimension::can_index_slice_with_strides(xs, &dim, &shape.strides)?;
         let strides = shape.strides.strides_for_dim(&dim);
-        unsafe { Ok(Self::new_(xs.as_mut_ptr().offset(-offset_from_ptr_to_memory(&dim, &strides)), dim, strides)) }
+        unsafe { Ok(Self::new_(xs.as_mut_ptr().add(offset_from_low_addr_ptr_to_logical_ptr(&dim, &strides)), dim, strides)) }
     }
 
     /// Create an `ArrayViewMut<A, D>` from shape information and a

tests/array-construct.rs

@@ -7,6 +7,7 @@
 
 use defmac::defmac;
 use ndarray::prelude::*;
+use ndarray::arr3;
 use ndarray::Zip;
 
 #[test]
@@ -164,6 +165,43 @@ fn test_ones() {
     assert_eq!(a, b);
 }
 
+#[test]
+fn test_from_shape_empty_with_neg_stride() {
+    // Issue #998, negative strides for an axis where it doesn't matter.
+    let s = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13];
+    let v = s[..12].to_vec();
+    let v_ptr = v.as_ptr();
+    let a = Array::from_shape_vec((2, 0, 2).strides((1, -4isize as usize, 2)), v).unwrap();
+    assert_eq!(a, arr3(&[[[0; 2]; 0]; 2]));
+    assert_eq!(a.as_ptr(), v_ptr);
+}
+
+#[test]
+fn test_from_shape_with_neg_stride() {
+    // Issue #998, negative strides for an axis where it doesn't matter.
+    let s = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13];
+    let v = s[..12].to_vec();
+    let v_ptr = v.as_ptr();
+    let a = Array::from_shape_vec((2, 1, 2).strides((1, -4isize as usize, 2)), v).unwrap();
+    assert_eq!(a, arr3(&[[[0, 2]],
+                         [[1, 3]]]));
+    assert_eq!(a.as_ptr(), v_ptr);
+}
+
+#[test]
+fn test_from_shape_2_2_2_with_neg_stride() {
+    // Issue #998, negative strides for an axis where it doesn't matter.
+    let s = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13];
+    let v = s[..12].to_vec();
+    let v_ptr = v.as_ptr();
+    let a = Array::from_shape_vec((2, 2, 2).strides((1, -4isize as usize, 2)), v).unwrap();
+    assert_eq!(a, arr3(&[[[4, 6],
+                          [0, 2]],
+                         [[5, 7],
+                          [1, 3]]]));
+    assert_eq!(a.as_ptr(), v_ptr.wrapping_add(4));
+}
+
 #[should_panic]
 #[test]
 fn deny_wraparound_zeros() {