Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] Mismatched and incorrect SAFETY comments in the `ARef::into_raw` doctest, alongside an unsound `RefCounted` trait implementation. --
--- Patch [6]: [PATCH v20 6/8] rust: Add missing SAFETY documentation for `ARef` example --- commit 1891eeeba9b2ed0af7d99f9518dcd85941d2518c Author: Oliver Mangold <[email protected]> rust: Add missing SAFETY documentation for ARef example This commit updates the rustdoc example for ARef to replace the "TODO" SAFETY comments with explanations for the RefCounted trait implementation and ARef::from_raw call. > diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs > --- a/rust/kernel/sync/aref.rs > +++ b/rust/kernel/sync/aref.rs > @@ -142,7 +142,9 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self { > /// > /// struct Empty {} > /// > - /// # // SAFETY: TODO. > + /// // SAFETY: The `RefCounted` implementation for `Empty` does not > count references and never > + /// // frees the underlying object. Thus we can act as owning an > increment on the refcount for > + /// // the object that we pass to the newly created `ARef`. > /// unsafe impl RefCounted for Empty { > /// fn inc_ref(&self) {} > /// unsafe fn dec_ref(_obj: NonNull<Self>) {} [Severity: Low] Are the SAFETY comments for the trait implementation and from_raw call swapped here? This comment justifies the caller requirement of ARef::from_raw (owning an increment), rather than the trait's own safety requirement (that increments keep the object alive). Additionally, does this explanation overlook the fact that the stack-allocated variable data will eventually go out of scope and be freed? If a developer copies this pattern and calls ARef::clone(), could it extend the pointer's lifetime beyond the stack frame without keeping the underlying object alive? > @@ -150,7 +152,7 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self { > /// > /// let mut data = Empty {}; > /// let ptr = NonNull::<Empty>::new(&mut data).unwrap(); > - /// # // SAFETY: TODO. > + /// // SAFETY: We keep `data` around longer than the `ARef`. > /// let data_ref: ARef<Empty> = unsafe { ARef::from_raw(ptr) }; [Severity: Low] Does this comment justify the object's lifetime (which is the responsibility of the RefCounted trait implementation) instead of addressing the specific safety requirement of ARef::from_raw? The from_raw method requires the caller to own a reference count increment. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
