Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Low] Incorrect use of `#[inline(always)]` instead of `#[inline]` for abstraction trait methods. - [Low] The commit message asserts that 'all repr types are normalized to unsigned integers', but pointer types use `*const c_void` as their representation. --
--- Patch [3]: [PATCH v3 03/16] rust: mem: add `AsRepr` and `AsReprMut` --- commit 0638c4f7e52f9eb87681654296d18ff15c916f15 Author: Gary Guo <[email protected]> rust: mem: add AsRepr and AsReprMut This commit adds AsRepr and AsReprMut traits to indicate that a type can be turned into a primitive for operations like atomics and I/O. It also provides implementations for booleans, pointers, and integers. > To avoid duplicating implementation, all repr types are normalized to > unsigned integers. [Severity: Low] This isn't a bug, but the commit message states that all repr types are normalized to unsigned integers. However, the implementations for *mut T and *const T use *const c_void as their representation instead of an unsigned integer. Should the commit message or the pointer implementations be updated to match? > diff --git a/rust/kernel/mem.rs b/rust/kernel/mem.rs > index a0901cbe1b2d6..5bce381d88952 100644 > --- a/rust/kernel/mem.rs > +++ b/rust/kernel/mem.rs [ ... ] > +pub unsafe trait AsRepr: Sized { > + /// Primitive representation of this type. > + type Repr; > + > + /// Convert from [`AsRepr::Repr`] to `Self`. > + /// > + /// # Safety > + /// > + /// `repr` must be a valid bit patern of `Self`. If `repr` is previously > obtained using > + /// [`AsRepr::into_repr`], then it will always be safe. > + #[inline(always)] > + unsafe fn from_repr_unchecked(repr: Self::Repr) -> Self { [Severity: Low] This isn't a bug, but is #[inline(always)] the intended attribute here? The Rust subsystem guidelines state that functions for abstractions should be annotated with #[inline] unless they use build_assert!() dependent on function parameters. > + // SAFETY: Per safety requirement of the trait. > + unsafe { transmute_unchecked(repr) } > + } > + > + /// Convert from `Self` to [`AsRepr::Repr`]. > + #[inline(always)] > + fn into_repr(this: Self) -> Self::Repr { [Severity: Low] This isn't a bug, but this method might also need to use #[inline] rather than #[inline(always)] for the same reason. > + // SAFETY: Per safety requirement of the trait. > + unsafe { transmute_unchecked(this) } > + } > +} > + > +/// Type that is bi-directionally transmutable with a primitive > representation. > +/// > +/// # Safety > +/// > +/// - [`Self`] must be [transmutable] from [`Self::Repr`]. > +/// - Note that [`Self::Repr`] must be [transmutable] from `Self` as well, > however that is a > +/// requirement of the [`AsRepr`] super trait already. > +/// > +/// [`transmutable`]: core::mem::transmute > +pub unsafe trait AsReprMut: AsRepr { > + /// Convert from [`AsRepr::Repr`] to `Self`. > + #[inline(always)] > + fn from_repr(repr: Self::Repr) -> Self { [Severity: Low] This isn't a bug, but would it be better to use #[inline] here to follow the abstraction guidelines? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
