Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Low] rust: test coverage regression: u32 dropped from atomic_arithmetic_tests - [Low] rust: dead code: unusable AtomicAdd implementations for u8 and u16 --
commit bff46a0936305602d6b5e98e2a29f7e77f8ccefc Author: Thomas Weißschuh <[email protected]> rust: sync: atomic: Add Atomic<u{8,16}> Add generic atomic support for small unsigned integers that have an AtomicImpl with the same size and alignment. Also extend the unit tests. > diff --git a/rust/kernel/sync/atomic/predefine.rs > b/rust/kernel/sync/atomic/predefine.rs > index 3d63f40791fa2..4c1fb5e08dc26 100644 > --- a/rust/kernel/sync/atomic/predefine.rs > +++ b/rust/kernel/sync/atomic/predefine.rs > @@ -113,6 +113,32 @@ fn rhs_into_delta(rhs: isize) -> isize_atomic_repr { > } > } > > +// SAFETY: `u8` and `i8` has the same size and alignment, and `u8` is > round-trip transmutable to > +// `i8`. > +unsafe impl super::AtomicType for u8 { > + type Repr = i8; > +} > + > +// SAFETY: The wrapping add result of two `i8`s is a valid `u8`. > +unsafe impl super::AtomicAdd<u8> for u8 { > + fn rhs_into_delta(rhs: u8) -> i8 { > + rhs as i8 > + } > +} [Severity: Low] Does this implementation for u8 result in unusable dead code? Since the Repr type for u8 is i8, and i8 lacks an AtomicArithmeticOps implementation, it seems impossible to call arithmetic methods like fetch_add or add on Atomic<u8>. > +// SAFETY: `u16` and `i16` has the same size and alignment, and `u16` is > round-trip transmutable to > +// `i16`. > +unsafe impl super::AtomicType for u16 { > + type Repr = i16; > +} > + > +// SAFETY: The wrapping add result of two `i16`s is a valid `u16`. > +unsafe impl super::AtomicAdd<u16> for u16 { > + fn rhs_into_delta(rhs: u16) -> i16 { > + rhs as i16 > + } > +} [Severity: Low] Does this implementation for u16 also result in unusable dead code? Similar to u8, the Repr type i16 lacks an AtomicArithmeticOps implementation, preventing the use of atomic arithmetic methods on Atomic<u16>. [ ... ] > @@ -263,7 +289,7 @@ fn atomic_cmpxchg_tests() { > > #[test] > fn atomic_arithmetic_tests() { > - for_each_type!(42 in [i32, i64, u32, u64, isize, usize] |v| { > + for_each_type!(42 in [i32, i64, u64, isize, usize] |v| { [Severity: Low] Is u32 intentionally dropped from the list of types tested here? This appears to cause a regression in test coverage for 32-bit unsigned atomic arithmetic operations. > let x = Atomic::new(v); > > assert_eq!(v, x.fetch_add(12, Full)); -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
