From 1e0cb7bc09b3de12bb71683832fcefc377ee1926 Mon Sep 17 00:00:00 2001 From: Julian Knodt Date: Sat, 16 Dec 2023 13:54:38 -0800 Subject: [PATCH] Fix Clippy Warnings (#1300) --- nalgebra-sparse/src/cs.rs | 11 ++++----- nalgebra-sparse/src/ops/impl_std_ops.rs | 2 +- nalgebra-sparse/src/ops/serial/pattern.rs | 28 +++++++++++++---------- nalgebra-sparse/src/pattern.rs | 20 +++++++++++----- src/base/construction_view.rs | 13 +++++++++++ src/base/dimension.rs | 4 ++++ src/base/indexing.rs | 7 ++++++ src/base/matrix.rs | 12 ++++++++++ src/base/matrix_view.rs | 20 +++++++++++----- src/base/norm.rs | 2 +- src/base/statistics.rs | 2 +- src/base/storage.rs | 18 +++++++++++++++ src/geometry/dual_quaternion.rs | 1 + src/geometry/point.rs | 12 ++++++++++ src/geometry/rotation.rs | 4 ++++ src/geometry/rotation_specialization.rs | 6 ++--- src/geometry/scale.rs | 4 ++++ src/geometry/scale_ops.rs | 8 +++---- src/linalg/svd2.rs | 2 +- 19 files changed, 135 insertions(+), 41 deletions(-) diff --git a/nalgebra-sparse/src/cs.rs b/nalgebra-sparse/src/cs.rs index 83dfdc01..30bdc9b8 100644 --- a/nalgebra-sparse/src/cs.rs +++ b/nalgebra-sparse/src/cs.rs @@ -1,4 +1,3 @@ -use std::mem::replace; use std::ops::Range; use num_traits::One; @@ -369,7 +368,7 @@ where if let Some(minor_indices) = lane { let count = minor_indices.len(); - let remaining = replace(&mut self.remaining_values, &mut []); + let remaining = std::mem::take(&mut self.remaining_values); let (values_in_lane, remaining) = remaining.split_at_mut(count); self.remaining_values = remaining; self.current_lane_idx += 1; @@ -578,7 +577,7 @@ where } else if sort { unreachable!("Internal error: Sorting currently not supported if no values are present."); } - if major_offsets.len() == 0 { + if major_offsets.is_empty() { return Err(SparseFormatError::from_kind_and_msg( SparseFormatErrorKind::InvalidStructure, "Number of offsets should be greater than 0.", @@ -624,12 +623,12 @@ where )); } - let minor_idx_in_lane = minor_indices.get(range_start..range_end).ok_or( + let minor_idx_in_lane = minor_indices.get(range_start..range_end).ok_or_else(|| { SparseFormatError::from_kind_and_msg( SparseFormatErrorKind::IndexOutOfBounds, "A major offset is out of bounds.", - ), - )?; + ) + })?; // We test for in-bounds, uniqueness and monotonicity at the same time // to ensure that we only visit each minor index once diff --git a/nalgebra-sparse/src/ops/impl_std_ops.rs b/nalgebra-sparse/src/ops/impl_std_ops.rs index 7363c9d9..63b6d22c 100644 --- a/nalgebra-sparse/src/ops/impl_std_ops.rs +++ b/nalgebra-sparse/src/ops/impl_std_ops.rs @@ -59,7 +59,7 @@ macro_rules! impl_sp_plus_minus { let mut result = $matrix_type::try_from_pattern_and_values(pattern, values) .unwrap(); $spadd_fn(T::zero(), &mut result, T::one(), Op::NoOp(&a)).unwrap(); - $spadd_fn(T::one(), &mut result, $factor * T::one(), Op::NoOp(&b)).unwrap(); + $spadd_fn(T::one(), &mut result, $factor, Op::NoOp(&b)).unwrap(); result }); diff --git a/nalgebra-sparse/src/ops/serial/pattern.rs b/nalgebra-sparse/src/ops/serial/pattern.rs index b73f3375..8806b4fc 100644 --- a/nalgebra-sparse/src/ops/serial/pattern.rs +++ b/nalgebra-sparse/src/ops/serial/pattern.rs @@ -125,18 +125,22 @@ fn iterate_union<'a>( ) -> impl Iterator + 'a { iter::from_fn(move || { if let (Some(a_item), Some(b_item)) = (sorted_a.first(), sorted_b.first()) { - let item = if a_item < b_item { - sorted_a = &sorted_a[1..]; - a_item - } else if b_item < a_item { - sorted_b = &sorted_b[1..]; - b_item - } else { - // Both lists contain the same element, advance both slices to avoid - // duplicate entries in the result - sorted_a = &sorted_a[1..]; - sorted_b = &sorted_b[1..]; - a_item + let item = match a_item.cmp(b_item) { + std::cmp::Ordering::Less => { + sorted_a = &sorted_a[1..]; + a_item + } + std::cmp::Ordering::Greater => { + sorted_b = &sorted_b[1..]; + b_item + } + std::cmp::Ordering::Equal => { + // Both lists contain the same element, advance both slices to avoid + // duplicate entries in the result + sorted_a = &sorted_a[1..]; + sorted_b = &sorted_b[1..]; + a_item + } }; Some(*item) } else if let Some(a_item) = sorted_a.first() { diff --git a/nalgebra-sparse/src/pattern.rs b/nalgebra-sparse/src/pattern.rs index 0bb654b5..803e7f2d 100644 --- a/nalgebra-sparse/src/pattern.rs +++ b/nalgebra-sparse/src/pattern.rs @@ -80,7 +80,7 @@ impl SparsityPattern { #[inline] #[must_use] pub fn major_dim(&self) -> usize { - assert!(self.major_offsets.len() > 0); + assert!(!self.major_offsets.is_empty()); self.major_offsets.len() - 1 } @@ -162,7 +162,7 @@ impl SparsityPattern { // We test for in-bounds, uniqueness and monotonicity at the same time // to ensure that we only visit each minor index once let mut iter = minor_indices.iter(); - let mut prev = None; + let mut prev: Option = None; while let Some(next) = iter.next().copied() { if next >= minor_dim { @@ -170,10 +170,10 @@ impl SparsityPattern { } if let Some(prev) = prev { - if prev > next { - return Err(NonmonotonicMinorIndices); - } else if prev == next { - return Err(DuplicateEntry); + match prev.cmp(&next) { + std::cmp::Ordering::Greater => return Err(NonmonotonicMinorIndices), + std::cmp::Ordering::Equal => return Err(DuplicateEntry), + std::cmp::Ordering::Less => {} } } prev = Some(next); @@ -195,6 +195,14 @@ impl SparsityPattern { /// /// Panics if the number of major offsets is not exactly one greater than the major dimension /// or if major offsets do not start with 0 and end with the number of minor indices. + /// + /// # Safety + /// + /// Assumes that the major offsets and indices adhere to the requirements of being a valid + /// sparsity pattern. + /// Specifically, that major offsets is monotonically increasing, and + /// `major_offsets[i]..major_offsets[i+1]` refers to a major lane in the sparsity pattern, + /// and `minor_indices[major_offsets[i]..major_offsets[i+1]]` is monotonically increasing. pub unsafe fn from_offset_and_indices_unchecked( major_dim: usize, minor_dim: usize, diff --git a/src/base/construction_view.rs b/src/base/construction_view.rs index 8584f5b2..21819e04 100644 --- a/src/base/construction_view.rs +++ b/src/base/construction_view.rs @@ -97,6 +97,8 @@ macro_rules! impl_constructors( } /// Creates, without bound checking, a new matrix view from the given data array. + /// # Safety + /// `data[start..start+rstride * cstride]` must be within bounds. #[inline] pub unsafe fn from_slice_unchecked(data: &'a [T], start: usize, $($args: usize),*) -> Self { Self::from_slice_generic_unchecked(data, start, $($gargs),*) @@ -113,6 +115,11 @@ macro_rules! impl_constructors( } /// Creates, without bound checking, a new matrix view with the specified strides from the given data array. + /// + /// # Safety + /// + /// `start`, `rstride`, and `cstride`, with the given matrix size will not index + /// outside of `data`. #[inline] pub unsafe fn from_slice_with_strides_unchecked(data: &'a [T], start: usize, $($args: usize,)* rstride: usize, cstride: usize) -> Self { Self::from_slice_with_strides_generic_unchecked(data, start, $($gargs,)* Dyn(rstride), Dyn(cstride)) @@ -257,6 +264,10 @@ macro_rules! impl_constructors_mut( } /// Creates, without bound checking, a new mutable matrix view from the given data array. + /// + /// # Safety + /// + /// `data[start..start+(R * C)]` must be within bounds. #[inline] pub unsafe fn from_slice_unchecked(data: &'a mut [T], start: usize, $($args: usize),*) -> Self { Self::from_slice_generic_unchecked(data, start, $($gargs),*) @@ -274,6 +285,8 @@ macro_rules! impl_constructors_mut( } /// Creates, without bound checking, a new mutable matrix view with the specified strides from the given data array. + /// # Safety + /// `data[start..start+rstride * cstride]` must be within bounds. #[inline] pub unsafe fn from_slice_with_strides_unchecked(data: &'a mut [T], start: usize, $($args: usize,)* rstride: usize, cstride: usize) -> Self { Self::from_slice_with_strides_generic_unchecked( diff --git a/src/base/dimension.rs b/src/base/dimension.rs index 11743dd8..0b72e188 100644 --- a/src/base/dimension.rs +++ b/src/base/dimension.rs @@ -68,6 +68,10 @@ impl IsNotStaticOne for Dyn {} /// Trait implemented by any type that can be used as a dimension. This includes type-level /// integers and `Dyn` (for dimensions not known at compile-time). +/// +/// # Safety +/// +/// Hoists integers to the type level, including binary operations. pub unsafe trait Dim: Any + Debug + Copy + PartialEq + Send + Sync { #[inline(always)] fn is() -> bool { diff --git a/src/base/indexing.rs b/src/base/indexing.rs index 48dd8fad..5a860fe0 100644 --- a/src/base/indexing.rs +++ b/src/base/indexing.rs @@ -519,6 +519,10 @@ impl> Matrix { /// Produces a view of the data at the given index, without doing /// any bounds checking. + /// + /// # Safety + /// + /// `index` must within bounds of the array. #[inline] #[must_use] pub unsafe fn get_unchecked<'a, I>(&'a self, index: I) -> I::Output @@ -530,6 +534,9 @@ impl> Matrix { /// Returns a mutable view of the data at the given index, without doing /// any bounds checking. + /// # Safety + /// + /// `index` must within bounds of the array. #[inline] #[must_use] pub unsafe fn get_unchecked_mut<'a, I>(&'a mut self, index: I) -> I::OutputMut diff --git a/src/base/matrix.rs b/src/base/matrix.rs index af5609df..36c95544 100644 --- a/src/base/matrix.rs +++ b/src/base/matrix.rs @@ -313,6 +313,10 @@ where impl Matrix { /// Creates a new matrix with the given data without statically checking that the matrix /// dimension matches the storage dimension. + /// + /// # Safety + /// + /// The storage dimension must match the given dimensions. #[inline(always)] pub const unsafe fn from_data_statically_unchecked(data: S) -> Matrix { Matrix { @@ -1194,6 +1198,10 @@ impl> Matrix { } /// Swaps two entries without bound-checking. + /// + /// # Safety + /// + /// Both `(r, c)` must have `r < nrows(), c < ncols()`. #[inline] pub unsafe fn swap_unchecked(&mut self, row_cols1: (usize, usize), row_cols2: (usize, usize)) { debug_assert!(row_cols1.0 < self.nrows() && row_cols1.1 < self.ncols()); @@ -1300,6 +1308,8 @@ impl> Matrix { impl> Vector { /// Gets a reference to the i-th element of this column vector without bound checking. + /// # Safety + /// `i` must be less than `D`. #[inline] #[must_use] pub unsafe fn vget_unchecked(&self, i: usize) -> &T { @@ -1311,6 +1321,8 @@ impl> Vector { impl> Vector { /// Gets a mutable reference to the i-th element of this column vector without bound checking. + /// # Safety + /// `i` must be less than `D`. #[inline] #[must_use] pub unsafe fn vget_unchecked_mut(&mut self, i: usize) -> &mut T { diff --git a/src/base/matrix_view.rs b/src/base/matrix_view.rs index fa6f8f00..ab3d68ce 100644 --- a/src/base/matrix_view.rs +++ b/src/base/matrix_view.rs @@ -43,6 +43,10 @@ macro_rules! view_storage_impl ( impl<'a, T, R: Dim, C: Dim, RStride: Dim, CStride: Dim> $T<'a, T, R, C, RStride, CStride> { /// Create a new matrix view without bounds checking and from a raw pointer. + /// + /// # Safety + /// + /// `*ptr` must point to memory that is valid `[T; R * C]`. #[inline] pub unsafe fn from_raw_parts(ptr: $Ptr, shape: (R, C), @@ -63,6 +67,11 @@ macro_rules! view_storage_impl ( // Dyn is arbitrary. It's just to be able to call the constructors with `Slice::` impl<'a, T, R: Dim, C: Dim> $T<'a, T, R, C, Dyn, Dyn> { /// Create a new matrix view without bounds checking. + /// + /// # Safety + /// + /// `storage` contains sufficient elements beyond `start + R * C` such that all + /// accesses are within bounds. #[inline] pub unsafe fn new_unchecked(storage: $SRef, start: (usize, usize), shape: (R, C)) -> $T<'a, T, R, C, S::RStride, S::CStride> @@ -75,6 +84,10 @@ macro_rules! view_storage_impl ( } /// Create a new matrix view without bounds checking. + /// + /// # Safety + /// + /// `strides` must be a valid stride indexing. #[inline] pub unsafe fn new_with_strides_unchecked(storage: $SRef, start: (usize, usize), @@ -128,12 +141,7 @@ impl<'a, T: Scalar, R: Dim, C: Dim, RStride: Dim, CStride: Dim> Clone { #[inline] fn clone(&self) -> Self { - Self { - ptr: self.ptr, - shape: self.shape, - strides: self.strides, - _phantoms: PhantomData, - } + *self } } diff --git a/src/base/norm.rs b/src/base/norm.rs index 63d20218..e9010273 100644 --- a/src/base/norm.rs +++ b/src/base/norm.rs @@ -525,7 +525,7 @@ where let (elt, basis) = vs[..i + 1].split_last_mut().unwrap(); for basis_element in &basis[..nbasis_elements] { - *elt -= &*basis_element * elt.dot(basis_element) + *elt -= basis_element * elt.dot(basis_element) } } diff --git a/src/base/statistics.rs b/src/base/statistics.rs index 6007f8c7..5bb05fd0 100644 --- a/src/base/statistics.rs +++ b/src/base/statistics.rs @@ -339,7 +339,7 @@ impl> Matrix { let mean = self.mean(); self.iter().cloned().fold(T::zero(), |acc, x| { - acc + (x.clone() - mean.clone()) * (x.clone() - mean.clone()) + acc + (x.clone() - mean.clone()) * (x - mean.clone()) }) / n_elements } } diff --git a/src/base/storage.rs b/src/base/storage.rs index dd335014..d5f1de61 100644 --- a/src/base/storage.rs +++ b/src/base/storage.rs @@ -32,6 +32,8 @@ pub type CStride = /// The trait shared by all matrix data storage. /// /// TODO: doc +/// # Safety +/// /// In generic code, it is recommended use the `Storage` trait bound instead. The `RawStorage` /// trait bound is generally used by code that needs to work with storages that contains /// `MaybeUninit` elements. @@ -129,6 +131,14 @@ pub unsafe trait RawStorage: Sized { } /// Trait shared by all matrix data storage that don’t contain any uninitialized elements. +/// +/// # Safety +/// +/// Note that `Self` must always have a number of elements compatible with the matrix length (given +/// by `R` and `C` if they are known at compile-time). For example, implementors of this trait +/// should **not** allow the user to modify the size of the underlying buffer with safe methods +/// (for example the `VecStorage::data_mut` method is unsafe because the user could change the +/// vector's size so that it no longer contains enough elements: this will lead to UB. pub unsafe trait Storage: RawStorage { /// Builds a matrix data storage that does not contain any reference. fn into_owned(self) -> Owned @@ -143,6 +153,8 @@ pub unsafe trait Storage: RawStorage { /// Trait implemented by matrix data storage that can provide a mutable access to its elements. /// +/// # Safety +/// /// In generic code, it is recommended use the `StorageMut` trait bound instead. The /// `RawStorageMut` trait bound is generally used by code that needs to work with storages that /// contains `MaybeUninit` elements. @@ -226,6 +238,10 @@ pub unsafe trait RawStorageMut: RawStorage { } /// Trait shared by all mutable matrix data storage that don’t contain any uninitialized elements. +/// +/// # Safety +/// +/// See safety note for `Storage`, `RawStorageMut`. pub unsafe trait StorageMut: Storage + RawStorageMut { @@ -241,6 +257,8 @@ where /// Marker trait indicating that a storage is stored contiguously in memory. /// +/// # Safety +/// /// The storage requirement means that for any value of `i` in `[0, nrows * ncols - 1]`, the value /// `.get_unchecked_linear` returns one of the matrix component. This trait is unsafe because /// failing to comply to this may cause Undefined Behaviors. diff --git a/src/geometry/dual_quaternion.rs b/src/geometry/dual_quaternion.rs index 83058032..3d073fff 100644 --- a/src/geometry/dual_quaternion.rs +++ b/src/geometry/dual_quaternion.rs @@ -320,6 +320,7 @@ where } impl DualQuaternion { + #[allow(clippy::wrong_self_convention)] fn to_vector(&self) -> OVector { self.as_ref().clone().into() } diff --git a/src/geometry/point.rs b/src/geometry/point.rs index e936c3da..5ccb6279 100644 --- a/src/geometry/point.rs +++ b/src/geometry/point.rs @@ -317,6 +317,10 @@ where } /// Gets a reference to i-th element of this point without bound-checking. + /// + /// # Safety + /// + /// `i` must be less than `self.len()`. #[inline] #[must_use] pub unsafe fn get_unchecked(&self, i: usize) -> &T { @@ -344,6 +348,10 @@ where } /// Gets a mutable reference to i-th element of this point without bound-checking. + /// + /// # Safety + /// + /// `i` must be less than `self.len()`. #[inline] #[must_use] pub unsafe fn get_unchecked_mut(&mut self, i: usize) -> &mut T { @@ -351,6 +359,10 @@ where } /// Swaps two entries without bound-checking. + /// + /// # Safety + /// + /// `i1` and `i2` must be less than `self.len()`. #[inline] pub unsafe fn swap_unchecked(&mut self, i1: usize, i2: usize) { self.coords.swap_unchecked((i1, 0), (i2, 0)) diff --git a/src/geometry/rotation.rs b/src/geometry/rotation.rs index 67a242cb..e2ea81ed 100644 --- a/src/geometry/rotation.rs +++ b/src/geometry/rotation.rs @@ -185,6 +185,10 @@ impl Rotation { } /// A mutable reference to the underlying matrix representation of this rotation. + /// + /// # Safety + /// + /// Invariants of the rotation matrix should not be violated. #[inline] #[deprecated(note = "Use `.matrix_mut_unchecked()` instead.")] pub unsafe fn matrix_mut(&mut self) -> &mut SMatrix { diff --git a/src/geometry/rotation_specialization.rs b/src/geometry/rotation_specialization.rs index eecc9b21..45db0352 100644 --- a/src/geometry/rotation_specialization.rs +++ b/src/geometry/rotation_specialization.rs @@ -1058,7 +1058,7 @@ impl Rotation3 { { let mut angles = [T::zero(); 3]; let eps = T::from_subset(&1e-7); - let _2 = T::from_subset(&2.0); + let two = T::from_subset(&2.0); if extrinsic { seq.reverse(); @@ -1090,7 +1090,7 @@ impl Rotation3 { -s1, c1, ); - let o_t = &c * self.matrix() * (c.transpose() * r1l); + let o_t = c * self.matrix() * (c.transpose() * r1l); angles[1] = o_t.m33.acos(); let safe1 = angles[1].abs() >= eps; @@ -1131,7 +1131,7 @@ impl Rotation3 { // dont adjust gimbal locked rotation if adjust && observable { angles[0] += T::pi(); - angles[1] = _2 * lambda - angles[1]; + angles[1] = two * lambda - angles[1]; angles[2] -= T::pi(); } diff --git a/src/geometry/scale.rs b/src/geometry/scale.rs index 86122b9d..2c09b70d 100644 --- a/src/geometry/scale.rs +++ b/src/geometry/scale.rs @@ -149,6 +149,10 @@ impl Scale { /// assert_eq!(t.inverse_unchecked() * t, Scale2::identity()); /// } /// ``` + /// + /// # Safety + /// + /// Should only be used if all scaling is known to be non-zero. #[inline] #[must_use] pub unsafe fn inverse_unchecked(&self) -> Scale diff --git a/src/geometry/scale_ops.rs b/src/geometry/scale_ops.rs index dc273fc8..b22f7b53 100644 --- a/src/geometry/scale_ops.rs +++ b/src/geometry/scale_ops.rs @@ -83,28 +83,28 @@ add_sub_impl!(Mul, mul, ClosedMul; (Const, U1), (Const, U1) -> (Const, U1) const D; for; where; self: &'a Scale, right: &'b SVector, Output = SVector; - SVector::from(self.vector.component_mul(right)); + self.vector.component_mul(right); 'a, 'b); add_sub_impl!(Mul, mul, ClosedMul; (Const, U1), (Const, U1) -> (Const, U1) const D; for; where; self: &'a Scale, right: SVector, Output = SVector; - SVector::from(self.vector.component_mul(&right)); + self.vector.component_mul(&right); 'a); add_sub_impl!(Mul, mul, ClosedMul; (Const, U1), (Const, U1) -> (Const, U1) const D; for; where; self: Scale, right: &'b SVector, Output = SVector; - SVector::from(self.vector.component_mul(right)); + self.vector.component_mul(right); 'b); add_sub_impl!(Mul, mul, ClosedMul; (Const, U1), (Const, U1) -> (Const, U1) const D; for; where; self: Scale, right: SVector, Output = SVector; - SVector::from(self.vector.component_mul(&right)); ); + self.vector.component_mul(&right); ); // Scale *= Scale add_sub_assign_impl!(MulAssign, mul_assign, ClosedMul; diff --git a/src/linalg/svd2.rs b/src/linalg/svd2.rs index 3bbd9a1b..f23fed50 100644 --- a/src/linalg/svd2.rs +++ b/src/linalg/svd2.rs @@ -21,7 +21,7 @@ pub fn svd_ordered2( // because q >= 0 and r >= 0. let sx = q.clone() + r.clone(); let sy = q - r; - let sy_sign = if sy < T::zero() { -one.clone() } else { one }; + let sy_sign = if sy < T::zero() { -one } else { one }; let singular_values = Vector2::new(sx, sy * sy_sign.clone()); if compute_u || compute_v {