From 50419d330be805f7120675632118167d36fc1940 Mon Sep 17 00:00:00 2001 From: bluss Date: Wed, 3 Feb 2021 13:01:25 +0100 Subject: [PATCH 1/3] FEAT: Add internal array builder methods from_data_ptr and with_strides_dim These methods will be used instead of the ArrayBase struct constructor expression. --- src/impl_internal_constructors.rs | 65 +++++++++++++++++++++++++++++++ src/impl_methods.rs | 2 +- src/lib.rs | 1 + 3 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 src/impl_internal_constructors.rs diff --git a/src/impl_internal_constructors.rs b/src/impl_internal_constructors.rs new file mode 100644 index 000000000..18e8e6874 --- /dev/null +++ b/src/impl_internal_constructors.rs @@ -0,0 +1,65 @@ +// Copyright 2021 bluss and ndarray developers. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use std::ptr::NonNull; + +use crate::imp_prelude::*; + +// internal "builder-like" methods +impl ArrayBase +where + S: RawData, +{ + /// Create an (initially) empty one-dimensional array from the given data and array head + /// pointer + /// + /// ## Safety + /// + /// The caller must ensure that the data storage and pointer is valid. + /// + /// See ArrayView::from_shape_ptr for general pointer validity documentation. + pub(crate) unsafe fn from_data_ptr(data: S, ptr: NonNull) -> Self { + let array = ArrayBase { + data: data, + ptr: ptr, + dim: Ix1(0), + strides: Ix1(1), + }; + debug_assert!(array.pointer_is_inbounds()); + array + } +} + +// internal "builder-like" methods +impl ArrayBase +where + S: RawData, + D: Dimension, +{ + + /// Set strides and dimension of the array to the new values + /// + /// The argument order with strides before dimensions is used because strides are often + /// computed as derived from the dimension. + /// + /// ## Safety + /// + /// The caller needs to ensure that the new strides and dimensions are correct + /// for the array data. + pub(crate) unsafe fn with_strides_dim(self, strides: E, dim: E) -> ArrayBase + where + E: Dimension + { + ArrayBase { + data: self.data, + ptr: self.ptr, + dim, + strides, + } + } +} diff --git a/src/impl_methods.rs b/src/impl_methods.rs index 7b6f3f6f5..227c2063a 100644 --- a/src/impl_methods.rs +++ b/src/impl_methods.rs @@ -1942,7 +1942,7 @@ where self.index_axis_move(axis, 0) } - fn pointer_is_inbounds(&self) -> bool { + pub(crate) fn pointer_is_inbounds(&self) -> bool { match self.data._data_slice() { None => { // special case for non-owned views diff --git a/src/lib.rs b/src/lib.rs index c71f09aaf..1d56dc254 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1488,6 +1488,7 @@ impl<'a, A> CowRepr<'a, A> { mod impl_clone; +mod impl_internal_constructors; mod impl_constructors; mod impl_methods; From bb6c4ab0e0507d804d478bb581ceb9326fae6886 Mon Sep 17 00:00:00 2001 From: bluss Date: Wed, 3 Feb 2021 13:23:41 +0100 Subject: [PATCH 2/3] FIX: Use array builder from_data_ptr(..).with_strides_dim() where possible This avoids using the naked ArrayBase { .. } and similar struct constructors, which should make it easier for us to change representation in the future if needed. The constructor methods are unsafe since they rely on the caller to assert certain invariants; this means that this change increases the number of `unsafe` code blocks in ndarray, but that's only correct since every ArrayBase construction is safety-critical w.r.t the validity of the pointer, dimensions and strides taken together. The resulting code is often easier to read - especially when we are only updating the strides and dimensions of an existing array or view. The .with_strides_dim() method requires the Dimension trait so that appropriate debug assertions can be added in this method if desired. This precludes us from using this method in the `Clone` impl, but that's just a minor flaw and the only exception. --- src/data_traits.rs | 18 ++--- src/impl_clone.rs | 1 + src/impl_constructors.rs | 9 +-- src/impl_cow.rs | 18 ++--- src/impl_methods.rs | 127 +++++++++++------------------- src/impl_raw_views.rs | 16 +--- src/impl_special_element_types.rs | 8 +- src/impl_views/constructors.rs | 14 +--- src/lib.rs | 8 +- 9 files changed, 77 insertions(+), 142 deletions(-) diff --git a/src/data_traits.rs b/src/data_traits.rs index 9e814a28c..2b59fcb8d 100644 --- a/src/data_traits.rs +++ b/src/data_traits.rs @@ -245,11 +245,10 @@ unsafe impl Data for OwnedArcRepr { { Self::ensure_unique(&mut self_); let data = Arc::try_unwrap(self_.data.0).ok().unwrap(); - ArrayBase { - data, - ptr: self_.ptr, - dim: self_.dim, - strides: self_.strides, + // safe because data is equivalent + unsafe { + ArrayBase::from_data_ptr(data, self_.ptr) + .with_strides_dim(self_.strides, self_.dim) } } @@ -544,11 +543,10 @@ unsafe impl<'a, A> Data for CowRepr<'a, A> { { match self_.data { CowRepr::View(_) => self_.to_owned(), - CowRepr::Owned(data) => ArrayBase { - data, - ptr: self_.ptr, - dim: self_.dim, - strides: self_.strides, + CowRepr::Owned(data) => unsafe { + // safe because the data is equivalent so ptr, dims remain valid + ArrayBase::from_data_ptr(data, self_.ptr) + .with_strides_dim(self_.strides, self_.dim) }, } } diff --git a/src/impl_clone.rs b/src/impl_clone.rs index 603708849..e2e111a12 100644 --- a/src/impl_clone.rs +++ b/src/impl_clone.rs @@ -11,6 +11,7 @@ use crate::RawDataClone; impl Clone for ArrayBase { fn clone(&self) -> ArrayBase { + // safe because `clone_with_ptr` promises to provide equivalent data and ptr unsafe { let (data, ptr) = self.data.clone_with_ptr(self.ptr); ArrayBase { diff --git a/src/impl_constructors.rs b/src/impl_constructors.rs index d556934d9..9af8048f6 100644 --- a/src/impl_constructors.rs +++ b/src/impl_constructors.rs @@ -470,12 +470,9 @@ where unsafe fn from_vec_dim_stride_unchecked(dim: D, strides: D, mut v: Vec) -> Self { // debug check for issues that indicates wrong use of this constructor debug_assert!(dimension::can_index_slice(&v, &dim, &strides).is_ok()); - ArrayBase { - ptr: nonnull_from_vec_data(&mut v).offset(-offset_from_ptr_to_memory(&dim, &strides)), - data: DataOwned::new(v), - strides, - dim, - } + + let ptr = nonnull_from_vec_data(&mut v).offset(-offset_from_ptr_to_memory(&dim, &strides)); + ArrayBase::from_data_ptr(DataOwned::new(v), ptr).with_strides_dim(strides, dim) } /// Create an array with uninitalized elements, shape `shape`. diff --git a/src/impl_cow.rs b/src/impl_cow.rs index 52a06bfa2..b880fd62c 100644 --- a/src/impl_cow.rs +++ b/src/impl_cow.rs @@ -33,11 +33,10 @@ where D: Dimension, { fn from(view: ArrayView<'a, A, D>) -> CowArray<'a, A, D> { - ArrayBase { - data: CowRepr::View(view.data), - ptr: view.ptr, - dim: view.dim, - strides: view.strides, + // safe because equivalent data + unsafe { + ArrayBase::from_data_ptr(CowRepr::View(view.data), view.ptr) + .with_strides_dim(view.strides, view.dim) } } } @@ -47,11 +46,10 @@ where D: Dimension, { fn from(array: Array) -> CowArray<'a, A, D> { - ArrayBase { - data: CowRepr::Owned(array.data), - ptr: array.ptr, - dim: array.dim, - strides: array.strides, + // safe because equivalent data + unsafe { + ArrayBase::from_data_ptr(CowRepr::Owned(array.data), array.ptr) + .with_strides_dim(array.strides, array.dim) } } } diff --git a/src/impl_methods.rs b/src/impl_methods.rs index 227c2063a..9eea42d15 100644 --- a/src/impl_methods.rs +++ b/src/impl_methods.rs @@ -242,11 +242,9 @@ where S: DataOwned, { let data = self.data.into_shared(); - ArrayBase { - data, - ptr: self.ptr, - dim: self.dim, - strides: self.strides, + // safe because: equivalent unmoved data, ptr and dims remain valid + unsafe { + ArrayBase::from_data_ptr(data, self.ptr).with_strides_dim(self.strides, self.dim) } } @@ -434,11 +432,9 @@ where *new_s = *s; }); - ArrayBase { - ptr: self.ptr, - data: self.data, - dim: new_dim, - strides: new_strides, + // safe because new dimension, strides allow access to a subset of old data + unsafe { + self.with_strides_dim(new_strides, new_dim) } } @@ -757,11 +753,9 @@ where self.collapse_axis(axis, index); let dim = self.dim.remove_axis(axis); let strides = self.strides.remove_axis(axis); - ArrayBase { - ptr: self.ptr, - data: self.data, - dim, - strides, + // safe because new dimension, strides allow access to a subset of old data + unsafe { + self.with_strides_dim(strides, dim) } } @@ -1244,11 +1238,9 @@ where /// Return the diagonal as a one-dimensional array. pub fn into_diag(self) -> ArrayBase { let (len, stride) = self.diag_params(); - ArrayBase { - data: self.data, - ptr: self.ptr, - dim: Ix1(len), - strides: Ix1(stride as Ix), + // safe because new len stride allows access to a subset of the current elements + unsafe { + self.with_strides_dim(Ix1(stride as Ix), Ix1(len)) } } @@ -1498,22 +1490,15 @@ where return Err(error::incompatible_shapes(&self.dim, &shape)); } // Check if contiguous, if not => copy all, else just adapt strides - if self.is_standard_layout() { - Ok(ArrayBase { - data: self.data, - ptr: self.ptr, - strides: shape.default_strides(), - dim: shape, - }) - } else if self.ndim() > 1 && self.raw_view().reversed_axes().is_standard_layout() { - Ok(ArrayBase { - data: self.data, - ptr: self.ptr, - strides: shape.fortran_strides(), - dim: shape, - }) - } else { - Err(error::from_kind(error::ErrorKind::IncompatibleLayout)) + unsafe { + // safe because arrays are contiguous and len is unchanged + if self.is_standard_layout() { + Ok(self.with_strides_dim(shape.default_strides(), shape)) + } else if self.ndim() > 1 && self.raw_view().reversed_axes().is_standard_layout() { + Ok(self.with_strides_dim(shape.fortran_strides(), shape)) + } else { + Err(error::from_kind(error::ErrorKind::IncompatibleLayout)) + } } } @@ -1554,11 +1539,9 @@ where // Check if contiguous, if not => copy all, else just adapt strides if self.is_standard_layout() { let cl = self.clone(); - ArrayBase { - data: cl.data, - ptr: cl.ptr, - strides: shape.default_strides(), - dim: shape, + // safe because array is contiguous and shape has equal number of elements + unsafe { + cl.with_strides_dim(shape.default_strides(), shape) } } else { let v = self.iter().cloned().collect::>(); @@ -1576,11 +1559,10 @@ where /// [3, 4]]).into_dyn(); /// ``` pub fn into_dyn(self) -> ArrayBase { - ArrayBase { - data: self.data, - ptr: self.ptr, - dim: self.dim.into_dyn(), - strides: self.strides.into_dyn(), + // safe because new dims equivalent + unsafe { + ArrayBase::from_data_ptr(self.data, self.ptr) + .with_strides_dim(self.strides.into_dyn(), self.dim.into_dyn()) } } @@ -1604,27 +1586,19 @@ where where D2: Dimension, { - if D::NDIM == D2::NDIM { - // safe because D == D2 - unsafe { + unsafe { + if D::NDIM == D2::NDIM { + // safe because D == D2 let dim = unlimited_transmute::(self.dim); let strides = unlimited_transmute::(self.strides); - return Ok(ArrayBase { - data: self.data, - ptr: self.ptr, - dim, - strides, - }); - } - } else if D::NDIM == None || D2::NDIM == None { // one is dynamic dim - if let Some(dim) = D2::from_dimension(&self.dim) { - if let Some(strides) = D2::from_dimension(&self.strides) { - return Ok(ArrayBase { - data: self.data, - ptr: self.ptr, - dim, - strides, - }); + return Ok(ArrayBase::from_data_ptr(self.data, self.ptr) + .with_strides_dim(strides, dim)); + } else if D::NDIM == None || D2::NDIM == None { // one is dynamic dim + // safe because dim, strides are equivalent under a different type + if let Some(dim) = D2::from_dimension(&self.dim) { + if let Some(strides) = D2::from_dimension(&self.strides) { + return Ok(self.with_strides_dim(strides, dim)); + } } } } @@ -1792,10 +1766,9 @@ where new_strides[new_axis] = strides[axis]; } } - ArrayBase { - dim: new_dim, - strides: new_strides, - ..self + // safe because axis invariants are checked above; they are a permutation of the old + unsafe { + self.with_strides_dim(new_strides, new_dim) } } @@ -1915,17 +1888,11 @@ where /// ***Panics*** if the axis is out of bounds. pub fn insert_axis(self, axis: Axis) -> ArrayBase { assert!(axis.index() <= self.ndim()); - let ArrayBase { - ptr, - data, - dim, - strides, - } = self; - ArrayBase { - ptr, - data, - dim: dim.insert_axis(axis), - strides: strides.insert_axis(axis), + // safe because a new axis of length one does not affect memory layout + unsafe { + let strides = self.strides.insert_axis(axis); + let dim = self.dim.insert_axis(axis); + self.with_strides_dim(strides, dim) } } diff --git a/src/impl_raw_views.rs b/src/impl_raw_views.rs index e3447cac5..2ac5c08c7 100644 --- a/src/impl_raw_views.rs +++ b/src/impl_raw_views.rs @@ -17,12 +17,8 @@ where /// meet all of the invariants of the `ArrayBase` type. #[inline] pub(crate) unsafe fn new(ptr: NonNull, dim: D, strides: D) -> Self { - RawArrayView { - data: RawViewRepr::new(), - ptr, - dim, - strides, - } + RawArrayView::from_data_ptr(RawViewRepr::new(), ptr) + .with_strides_dim(strides, dim) } unsafe fn new_(ptr: *const A, dim: D, strides: D) -> Self { @@ -163,12 +159,8 @@ where /// meet all of the invariants of the `ArrayBase` type. #[inline] pub(crate) unsafe fn new(ptr: NonNull, dim: D, strides: D) -> Self { - RawArrayViewMut { - data: RawViewRepr::new(), - ptr, - dim, - strides, - } + RawArrayViewMut::from_data_ptr(RawViewRepr::new(), ptr) + .with_strides_dim(strides, dim) } unsafe fn new_(ptr: *mut A, dim: D, strides: D) -> Self { diff --git a/src/impl_special_element_types.rs b/src/impl_special_element_types.rs index e41177dbf..bf4384e42 100644 --- a/src/impl_special_element_types.rs +++ b/src/impl_special_element_types.rs @@ -40,12 +40,6 @@ where // "transmute" from storage of MaybeUninit to storage of A let data = S::data_subst(data); let ptr = ptr.cast::(); - - ArrayBase { - data, - ptr, - dim, - strides, - } + ArrayBase::from_data_ptr(data, ptr).with_strides_dim(strides, dim) } } diff --git a/src/impl_views/constructors.rs b/src/impl_views/constructors.rs index c6e5f9988..a133ee1d1 100644 --- a/src/impl_views/constructors.rs +++ b/src/impl_views/constructors.rs @@ -230,12 +230,7 @@ where assert!(is_aligned(ptr.as_ptr()), "The pointer must be aligned."); dimension::max_abs_offset_check_overflow::(&dim, &strides).unwrap(); } - ArrayView { - data: ViewRepr::new(), - ptr, - dim, - strides, - } + ArrayView::from_data_ptr(ViewRepr::new(), ptr).with_strides_dim(strides, dim) } /// Unsafe because: `ptr` must be valid for the given dimension and strides. @@ -258,12 +253,7 @@ where assert!(is_aligned(ptr.as_ptr()), "The pointer must be aligned."); dimension::max_abs_offset_check_overflow::(&dim, &strides).unwrap(); } - ArrayViewMut { - data: ViewRepr::new(), - ptr, - dim, - strides, - } + ArrayViewMut::from_data_ptr(ViewRepr::new(), ptr).with_strides_dim(strides, dim) } /// Create a new `ArrayView` diff --git a/src/lib.rs b/src/lib.rs index 1d56dc254..0c7caf735 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1565,11 +1565,9 @@ where fn try_remove_axis(self, axis: Axis) -> ArrayBase { let d = self.dim.try_remove_axis(axis); let s = self.strides.try_remove_axis(axis); - ArrayBase { - ptr: self.ptr, - data: self.data, - dim: d, - strides: s, + // safe because new dimension, strides allow access to a subset of old data + unsafe { + self.with_strides_dim(s, d) } } From 0d4272f7850181a9e5a3fcf25b9cf450a13f63d3 Mon Sep 17 00:00:00 2001 From: bluss Date: Thu, 4 Feb 2021 00:27:01 +0100 Subject: [PATCH 3/3] FIX: Add debug assertion in with_strides_dim Co-authored-by: Jim Turner --- src/impl_internal_constructors.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/impl_internal_constructors.rs b/src/impl_internal_constructors.rs index 18e8e6874..0ed60a622 100644 --- a/src/impl_internal_constructors.rs +++ b/src/impl_internal_constructors.rs @@ -55,6 +55,7 @@ where where E: Dimension { + debug_assert_eq!(strides.ndim(), dim.ndim()); ArrayBase { data: self.data, ptr: self.ptr,