Change ArrayBase.ptr to NonNull type (#683)

* Switch ArrayBase.ptr to be NonNull * Handle NonNull ArrayBase.ptr in linalg * Reorder impl_clone imports * Switch entirely to NonNull pointers in data_traits * Use unchecked NonNull creation in RawArrayViews * MAINT: Add dependency on rawpointer rawpointer 0.2 implements offset methods for NonNull<T>, which will be useful for us. This was already a dev-dependency, and also already a regular transitive dependency (from matrixmultiply). * Use NonNull::offset method and factor out nonnull constructors Add two constructors that suffice for ndarray: - Get NonNull<T> from Vec<T>'s buffer pointer - Get NonNull<T> from raw pointer, and check if it's nonnull with a debug assertion In all other cases we can use .offset() on the NonNull (method from rawpointer 0.2, so we don't have to explicit roundtrip through raw pointers when we offset NonNull<T>. For safety, see this documentation in the rawpointer crate, which says why using .offset() on NonNull is safe. NonNull<T> supports the same offsetting methods under the same safety constraints as the other raw pointer implementations. There is no difference - both when offsetting *mut T and NonNull<T>, the offset is only well defined if we remain inside the same object or one-past the end, and we can never land in a null pointer while obeying those rules.
rust-ndarray · Sep 9, 2019 · b98c844 · b98c844
1 parent c9c6112
commit b98c844
Show file tree

Hide file tree

Showing 17 changed files with 131 additions and 89 deletions.
diff --git a/Cargo.toml b/Cargo.toml
@@ -44,11 +44,11 @@ blas-src = { version = "0.2.0", optional = true, default-features = false }
 matrixmultiply = { version = "0.2.0" }
 # Use via the `serde-1` crate feature!
 serde = { version = "1.0", optional = true }
+rawpointer = { version = "0.2" }
 
 [dev-dependencies]
 defmac = "0.2"
 quickcheck = { version = "0.8", default-features = false }
-rawpointer = "0.1"
 approx = "0.3.2"
 itertools = { version = "0.8.0", default-features = false, features = ["use_std"] }
 

diff --git a/src/arraytraits.rs b/src/arraytraits.rs
@@ -50,7 +50,7 @@ where
     fn index(&self, index: I) -> &S::Elem {
         debug_bounds_check!(self, index);
         unsafe {
-            &*self.ptr.offset(
+            &*self.ptr.as_ptr().offset(
                 index
                     .index_checked(&self.dim, &self.strides)
                     .unwrap_or_else(|| array_out_of_bounds()),

diff --git a/src/data_traits.rs b/src/data_traits.rs
@@ -8,7 +8,10 @@
 
 //! The data (inner representation) traits for ndarray
 
+use crate::extension::nonnull::nonnull_from_vec_data;
+use rawpointer::PointerExt;
 use std::mem::{self, size_of};
+use std::ptr::NonNull;
 use std::sync::Arc;
 
 use crate::{
@@ -67,14 +70,14 @@ pub unsafe trait RawDataMut: RawData {
 pub unsafe trait RawDataClone: RawData {
     #[doc(hidden)]
     /// Unsafe because, `ptr` must point inside the current storage.
-    unsafe fn clone_with_ptr(&self, ptr: *mut Self::Elem) -> (Self, *mut Self::Elem);
+    unsafe fn clone_with_ptr(&self, ptr: NonNull<Self::Elem>) -> (Self, NonNull<Self::Elem>);
 
     #[doc(hidden)]
     unsafe fn clone_from_with_ptr(
         &mut self,
         other: &Self,
-        ptr: *mut Self::Elem,
-    ) -> *mut Self::Elem {
+        ptr: NonNull<Self::Elem>,
+    ) -> NonNull<Self::Elem> {
         let (data, ptr) = other.clone_with_ptr(ptr);
         *self = data;
         ptr
@@ -148,7 +151,7 @@ unsafe impl<A> RawData for RawViewRepr<*const A> {
 }
 
 unsafe impl<A> RawDataClone for RawViewRepr<*const A> {
-    unsafe fn clone_with_ptr(&self, ptr: *mut Self::Elem) -> (Self, *mut Self::Elem) {
+    unsafe fn clone_with_ptr(&self, ptr: NonNull<Self::Elem>) -> (Self, NonNull<Self::Elem>) {
         (*self, ptr)
     }
 }
@@ -177,7 +180,7 @@ unsafe impl<A> RawDataMut for RawViewRepr<*mut A> {
 }
 
 unsafe impl<A> RawDataClone for RawViewRepr<*mut A> {
-    unsafe fn clone_with_ptr(&self, ptr: *mut Self::Elem) -> (Self, *mut Self::Elem) {
+    unsafe fn clone_with_ptr(&self, ptr: NonNull<Self::Elem>) -> (Self, NonNull<Self::Elem>) {
         (*self, ptr)
     }
 }
@@ -217,13 +220,13 @@ where
         let rcvec = &mut self_.data.0;
         let a_size = mem::size_of::<A>() as isize;
         let our_off = if a_size != 0 {
-            (self_.ptr as isize - rcvec.as_ptr() as isize) / a_size
+            (self_.ptr.as_ptr() as isize - rcvec.as_ptr() as isize) / a_size
         } else {
             0
         };
         let rvec = Arc::make_mut(rcvec);
         unsafe {
-            self_.ptr = rvec.as_mut_ptr().offset(our_off);
+            self_.ptr = nonnull_from_vec_data(rvec).offset(our_off);
         }
     }
 
@@ -252,7 +255,7 @@ unsafe impl<A> Data for OwnedArcRepr<A> {
 unsafe impl<A> DataMut for OwnedArcRepr<A> where A: Clone {}
 
 unsafe impl<A> RawDataClone for OwnedArcRepr<A> {
-    unsafe fn clone_with_ptr(&self, ptr: *mut Self::Elem) -> (Self, *mut Self::Elem) {
+    unsafe fn clone_with_ptr(&self, ptr: NonNull<Self::Elem>) -> (Self, NonNull<Self::Elem>) {
         // pointer is preserved
         (self.clone(), ptr)
     }
@@ -298,11 +301,12 @@ unsafe impl<A> RawDataClone for OwnedRepr<A>
 where
     A: Clone,
 {
-    unsafe fn clone_with_ptr(&self, ptr: *mut Self::Elem) -> (Self, *mut Self::Elem) {
+    unsafe fn clone_with_ptr(&self, ptr: NonNull<Self::Elem>) -> (Self, NonNull<Self::Elem>) {
         let mut u = self.clone();
-        let mut new_ptr = u.0.as_mut_ptr();
+        let mut new_ptr = nonnull_from_vec_data(&mut u.0);
         if size_of::<A>() != 0 {
-            let our_off = (ptr as isize - self.0.as_ptr() as isize) / mem::size_of::<A>() as isize;
+            let our_off =
+                (ptr.as_ptr() as isize - self.0.as_ptr() as isize) / mem::size_of::<A>() as isize;
             new_ptr = new_ptr.offset(our_off);
         }
         (u, new_ptr)
@@ -311,15 +315,15 @@ where
     unsafe fn clone_from_with_ptr(
         &mut self,
         other: &Self,
-        ptr: *mut Self::Elem,
-    ) -> *mut Self::Elem {
+        ptr: NonNull<Self::Elem>,
+    ) -> NonNull<Self::Elem> {
         let our_off = if size_of::<A>() != 0 {
-            (ptr as isize - other.0.as_ptr() as isize) / mem::size_of::<A>() as isize
+            (ptr.as_ptr() as isize - other.0.as_ptr() as isize) / mem::size_of::<A>() as isize
         } else {
             0
         };
         self.0.clone_from(&other.0);
-        self.0.as_mut_ptr().offset(our_off)
+        nonnull_from_vec_data(&mut self.0).offset(our_off)
     }
 }
 
@@ -342,7 +346,7 @@ unsafe impl<'a, A> Data for ViewRepr<&'a A> {
 }
 
 unsafe impl<'a, A> RawDataClone for ViewRepr<&'a A> {
-    unsafe fn clone_with_ptr(&self, ptr: *mut Self::Elem) -> (Self, *mut Self::Elem) {
+    unsafe fn clone_with_ptr(&self, ptr: NonNull<Self::Elem>) -> (Self, NonNull<Self::Elem>) {
         (*self, ptr)
     }
 }
@@ -469,7 +473,7 @@ unsafe impl<'a, A> RawDataClone for CowRepr<'a, A>
 where
     A: Clone,
 {
-    unsafe fn clone_with_ptr(&self, ptr: *mut Self::Elem) -> (Self, *mut Self::Elem) {
+    unsafe fn clone_with_ptr(&self, ptr: NonNull<Self::Elem>) -> (Self, NonNull<Self::Elem>) {
         match self {
             CowRepr::View(view) => {
                 let (new_view, ptr) = view.clone_with_ptr(ptr);
@@ -486,8 +490,8 @@ where
     unsafe fn clone_from_with_ptr(
         &mut self,
         other: &Self,
-        ptr: *mut Self::Elem,
-    ) -> *mut Self::Elem {
+        ptr: NonNull<Self::Elem>,
+    ) -> NonNull<Self::Elem> {
         match (&mut *self, other) {
             (CowRepr::View(self_), CowRepr::View(other)) => self_.clone_from_with_ptr(other, ptr),
             (CowRepr::Owned(self_), CowRepr::Owned(other)) => self_.clone_from_with_ptr(other, ptr),

diff --git a/src/extension.rs b/src/extension.rs
@@ -0,0 +1,11 @@
+// Copyright 2019 bluss and ndarray developers.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+//! Extension traits and utility functions for types from outside ndarray
+
+pub(crate) mod nonnull;
diff --git a/src/extension/nonnull.rs b/src/extension/nonnull.rs
@@ -0,0 +1,18 @@
+use std::ptr::NonNull;
+
+/// Return a NonNull<T> pointer to the vector's data
+pub(crate) fn nonnull_from_vec_data<T>(v: &mut Vec<T>) -> NonNull<T> {
+    // this pointer is guaranteed to be non-null
+    unsafe { NonNull::new_unchecked(v.as_mut_ptr()) }
+}
+
+/// Converts `ptr` to `NonNull<T>`
+///
+/// Safety: `ptr` *must* be non-null.
+/// This is checked with a debug assertion, and will panic if this is not true,
+/// but treat this as an unconditional conversion.
+#[inline]
+pub(crate) unsafe fn nonnull_debug_checked_from_ptr<T>(ptr: *mut T) -> NonNull<T> {
+    debug_assert!(!ptr.is_null());
+    NonNull::new_unchecked(ptr)
+}
diff --git a/src/impl_clone.rs b/src/impl_clone.rs
@@ -5,6 +5,7 @@
 // <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
+
 use crate::imp_prelude::*;
 use crate::RawDataClone;
 

diff --git a/src/impl_constructors.rs b/src/impl_constructors.rs
@@ -16,6 +16,7 @@ use num_traits::{Float, One, Zero};
 
 use crate::dimension;
 use crate::error::{self, ShapeError};
+use crate::extension::nonnull::nonnull_from_vec_data;
 use crate::imp_prelude::*;
 use crate::indexes;
 use crate::indices;
@@ -422,7 +423,7 @@ where
         // debug check for issues that indicates wrong use of this constructor
         debug_assert!(dimension::can_index_slice(&v, &dim, &strides).is_ok());
         ArrayBase {
-            ptr: v.as_mut_ptr(),
+            ptr: nonnull_from_vec_data(&mut v),
             data: DataOwned::new(v),
             strides,
             dim,

diff --git a/src/impl_methods.rs b/src/impl_methods.rs
@@ -11,6 +11,7 @@ use std::ptr as std_ptr;
 use std::slice;
 
 use itertools::{izip, zip};
+use rawpointer::PointerExt;
 
 use crate::imp_prelude::*;
 
@@ -138,7 +139,7 @@ where
         S: Data,
     {
         debug_assert!(self.pointer_is_inbounds());
-        unsafe { ArrayView::new_(self.ptr, self.dim.clone(), self.strides.clone()) }
+        unsafe { ArrayView::new_(self.ptr.as_ptr(), self.dim.clone(), self.strides.clone()) }
     }
 
     /// Return a read-write view of the array
@@ -147,7 +148,7 @@ where
         S: DataMut,
     {
         self.ensure_unique();
-        unsafe { ArrayViewMut::new_(self.ptr, self.dim.clone(), self.strides.clone()) }
+        unsafe { ArrayViewMut::new_(self.ptr.as_ptr(), self.dim.clone(), self.strides.clone()) }
     }
 
     /// Return an uniquely owned copy of the array.
@@ -506,7 +507,7 @@ where
         let ptr = self.ptr;
         index
             .index_checked(&self.dim, &self.strides)
-            .map(move |offset| unsafe { ptr.offset(offset) as *const _ })
+            .map(move |offset| unsafe { ptr.as_ptr().offset(offset) as *const _ })
     }
 
     /// Return a mutable reference to the element at `index`, or return `None`
@@ -545,7 +546,7 @@ where
     {
         arraytraits::debug_bounds_check(self, &index);
         let off = index.index_unchecked(&self.strides);
-        &*self.ptr.offset(off)
+        &*self.ptr.as_ptr().offset(off)
     }
 
     /// Perform *unchecked* array indexing.
@@ -563,7 +564,7 @@ where
         debug_assert!(self.data.is_unique());
         arraytraits::debug_bounds_check(self, &index);
         let off = index.index_unchecked(&self.strides);
-        &mut *self.ptr.offset(off)
+        &mut *self.ptr.as_ptr().offset(off)
     }
 
     /// Swap elements at indices `index1` and `index2`.
@@ -599,7 +600,10 @@ where
         arraytraits::debug_bounds_check(self, &index2);
         let off1 = index1.index_unchecked(&self.strides);
         let off2 = index2.index_unchecked(&self.strides);
-        std_ptr::swap(self.ptr.offset(off1), self.ptr.offset(off2));
+        std_ptr::swap(
+            self.ptr.as_ptr().offset(off1),
+            self.ptr.as_ptr().offset(off2),
+        );
     }
 
     // `get` for zero-dimensional arrays
@@ -1293,7 +1297,7 @@ where
     /// where *d* is `self.ndim()`.
     #[inline(always)]
     pub fn as_ptr(&self) -> *const A {
-        self.ptr
+        self.ptr.as_ptr() as *const A
     }
 
     /// Return a mutable pointer to the first element in the array.
@@ -1303,13 +1307,13 @@ where
         S: RawDataMut,
     {
         self.try_ensure_unique(); // for RcArray
-        self.ptr
+        self.ptr.as_ptr()
     }
 
     /// Return a raw view of the array.
     #[inline]
     pub fn raw_view(&self) -> RawArrayView<A, D> {
-        unsafe { RawArrayView::new_(self.ptr, self.dim.clone(), self.strides.clone()) }
+        unsafe { RawArrayView::new_(self.ptr.as_ptr(), self.dim.clone(), self.strides.clone()) }
     }
 
     /// Return a raw mutable view of the array.
@@ -1319,7 +1323,7 @@ where
         S: RawDataMut,
     {
         self.try_ensure_unique(); // for RcArray
-        unsafe { RawArrayViewMut::new_(self.ptr, self.dim.clone(), self.strides.clone()) }
+        unsafe { RawArrayViewMut::new_(self.ptr.as_ptr(), self.dim.clone(), self.strides.clone()) }
     }
 
     /// Return the array’s data as a slice, if it is contiguous and in standard order.
@@ -1332,7 +1336,7 @@ where
         S: Data,
     {
         if self.is_standard_layout() {
-            unsafe { Some(slice::from_raw_parts(self.ptr, self.len())) }
+            unsafe { Some(slice::from_raw_parts(self.ptr.as_ptr(), self.len())) }
         } else {
             None
         }
@@ -1346,7 +1350,7 @@ where
     {
         if self.is_standard_layout() {
             self.ensure_unique();
-            unsafe { Some(slice::from_raw_parts_mut(self.ptr, self.len())) }
+            unsafe { Some(slice::from_raw_parts_mut(self.ptr.as_ptr(), self.len())) }
         } else {
             None
         }
@@ -1364,7 +1368,7 @@ where
         S: Data,
     {
         if self.is_contiguous() {
-            unsafe { Some(slice::from_raw_parts(self.ptr, self.len())) }
+            unsafe { Some(slice::from_raw_parts(self.ptr.as_ptr(), self.len())) }
         } else {
             None
         }
@@ -1378,7 +1382,7 @@ where
     {
         if self.is_contiguous() {
             self.ensure_unique();
-            unsafe { Some(slice::from_raw_parts_mut(self.ptr, self.len())) }
+            unsafe { Some(slice::from_raw_parts_mut(self.ptr.as_ptr(), self.len())) }
         } else {
             None
         }
@@ -1616,7 +1620,7 @@ where
             Some(st) => st,
             None => return None,
         };
-        unsafe { Some(ArrayView::new_(self.ptr, dim, broadcast_strides)) }
+        unsafe { Some(ArrayView::new_(self.ptr.as_ptr(), dim, broadcast_strides)) }
     }
 
     /// Swap axes `ax` and `bx`.
@@ -1843,7 +1847,7 @@ where
             Some(slc) => {
                 let ptr = slc.as_ptr() as *mut A;
                 let end = unsafe { ptr.add(slc.len()) };
-                self.ptr >= ptr && self.ptr <= end
+                self.ptr.as_ptr() >= ptr && self.ptr.as_ptr() <= end
             }
         }
     }

diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs
@@ -29,7 +29,7 @@ impl<A> Array<A, Ix0> {
             // (This is necessary because the element in the array might not be
             // the first element in the `Vec`, such as if the array was created
             // by `array![1, 2, 3, 4].slice_move(s![2])`.)
-            let first = self.ptr as usize;
+            let first = self.ptr.as_ptr() as usize;
             let base = self.data.0.as_ptr() as usize;
             let index = (first - base) / size;
             debug_assert_eq!((first - base) % size, 0);