Re: [PATCH v15 2/3] rust: xarray: Add an abstraction for XArray

[Boqun Feng <boqun.feng@gmail.com>]

On Thu, Feb 06, 2025 at 01:21:35PM -0500, Tamir Duberstein wrote:
> Hi Boqun,
> 
> On Thu, Feb 6, 2025 at 12:18 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > Hi Tamir,
> >
> > This looks good to me overall, a few comments below:
> >
> > On Thu, Feb 06, 2025 at 11:24:44AM -0500, Tamir Duberstein wrote:
> > [...]
> > > +impl<'a, T: ForeignOwnable> Guard<'a, T> {
> > [...]
> > > +    /// Loads an entry from the array.
> > > +    ///
> > > +    /// Returns the entry at the given index.
> > > +    pub fn get(&self, index: usize) -> Option<T::Borrowed<'_>> {
> > > +        self.load(index, |ptr| {
> > > +            // SAFETY: `ptr` came from `T::into_foreign`.
> > > +            unsafe { T::borrow(ptr.as_ptr()) }
> > > +        })
> > > +    }
> > > +
> > > +    /// Loads an entry from the array.
> >
> > Nit: firstly, this function has the same description of `get()`, also
> > I would prefer something like "Returns a [`T::Borrowed`] of the object
> > at `index`" rather then "Loads an entry from the array", thoughts?
> 
> I was trying to avoid repeating the signature in the comment. In other
> words I was trying to write a comment that wouldn't have to change if
> the signature (but not the semantics) of the function changed. Since

Understood. However, I think doc comments and function signatures (and
name) can have the overlapped information, because they are for
different users. Surely a developer who already knows what XArray is
will make a good guess on what `get()` and `get_mut()` do, but it won't
hurt to have the doc comments double-confirming the guess. Besides there
could also be someone who is not that familiar with XArray and would
like to seek the information from the doc comments at first, then having
a more precise description would be helpful.

> the difference between `get` and `get_mut` is completely described in
> the type system, the two functions got the same comment. Shall I
> change it?
> 

Your call ;-) It's a nitpicking after all, and you're the maintainer.
However, I do want to make the point that being a bit more comprehensive
won't hurt when providing an API.

Regards,
Boqun

> > > +    ///
> > > +    /// Returns the entry at the given index.
> > > +    pub fn get_mut(&mut self, index: usize) -> Option<T::BorrowedMut<'_>> {
> > > +        self.load(index, |ptr| {
> > > +            // SAFETY: `ptr` came from `T::into_foreign`.
> > > +            unsafe { T::borrow_mut(ptr.as_ptr()) }
> > > +        })
> > > +    }
> > > +
> > > +    /// Erases an entry from the array.
> >
> > Nit: s/Erases/Removes?
> 
> Will change. I used "erase" because that's the verb used in the C
> function name but named it "remove" because that's the verb used in
> the Rust standard library. The result is neither here nor there :)
> 
> >
> > > +    ///
> > > +    /// Returns the entry which was previously at the given index.
> > > +    pub fn remove(&mut self, index: usize) -> Option<T> {
> > > +        // SAFETY: `self.xa.xa` is always valid by the type invariant.
> > > +        //
> > > +        // SAFETY: The caller holds the lock.
> > > +        let ptr = unsafe { bindings::__xa_erase(self.xa.xa.get(), index) }.cast();
> > > +        // SAFETY: `ptr` is either NULL or came from `T::into_foreign`.
> >
> > SAFETY comment here needs to mention why there is no alive `T::Borrowed`
> > or `T::BorrowedMut` out there per the safety requirement.
> 
> Will do.
> 
> > Regards,
> > Boqun
> >
> > > +        unsafe { T::try_from_foreign(ptr) }
> > > +    }
> > > +
> > [...]
> 
> Thanks for the review!