Hi Maíra,
Thanks for keeping up the work on this! A quick "nit review" on docs/comments.
On Fri, Feb 9, 2024 at 11:32 PM Maíra Canal <mcanal@xxxxxxxxxx> wrote:
>
> +//! C header: [`include/linux/xarray.h`](../../include/linux/xarray.h)
This can be migrated to the new `srctree/` notation:
+//! C header: [`include/linux/xarray.h`](srctree/include/linux/xarray.h)
> +/// Flags passed to `XArray::new` to configure the `XArray`.
Please use intra-doc links where possible, e.g. this could be:
/// Flags passed to [`XArray::new`] to configure the [`XArray`].
Sometimes you may need to help `rustdoc` a bit -- you can check how it
is done in other places for those that may not work. Of course, if a
particular instance gets too involved/hard to read for plain text
reading, then you can ignore it.
(Same for other instances, e.g. I see `None`, `Vec<Option<T>>`,
`Arc<T>`, `ForeignOwnable`, `Deref`..., also methods like
`into_foreign` etc.).
> +/// This is similar to a `Vec<Option<T>>`, but more efficient when there are holes in the
> +/// index space, and can be efficiently grown.
This line seems wrapped differently than the others nearby -- in
general, please try to keep comments at 100 columns if possible (or at
least try to be consistent within the same file).
> +/// INVARIANT: All pointers stored in the array are pointers obtained by
> +/// calling `T::into_foreign` or are NULL pointers. By using the pin-init
> +/// initialization, `self.xa` is always an initialized and valid XArray.
Shouldn't this be in an `# Invariants` section instead?
> +/// `Guard` holds a reference (`self.0`) to the underlying value owned by the
> +/// `XArray` (`self.1`) with its lock held.
Should we use "pointer" here?
> + // SAFETY: By the type invariant, we own the XArray lock, so we must
> + // unlock it here.
Please also use Markdown in normal comments too (like you do elsewhere).
(Same for other instances).
> + // Consider it a success anyway, not much we can do
Period at the end.
> +///
> +/// # Examples
> +///
Is something missing here at the top of these docs? Or what is the intention?
> +/// use kernel::prelude::*;
This line can be removed -- it is implicitly added in examples.
> +/// let foo = Arc::try_new(Foo { a : 1, b: 2 }).expect("Unable to allocate Foo");
Please consider using `?` instead to simplify the example, you will
need to add a line at the end for that (see e.g. `arc.rs`):
/// # Ok::<(), Error>(())
> + /// The type is `unsigned long`, which is always the same as `usize` in
> + /// the kernel. Therefore, we can use this method to convert between those.
This comment sounds like an implementation detail, rather than the
documentation of the function.
> + let mut index: core::ffi::c_ulong = 0;
This is `use`d above, but here the full path is given.
(Same for other instances).
> +// SAFETY: XArray is thread-safe and all mutation operations are internally locked.
Ideally we use different comments for each `unsafe impl` and we
explain the rationale for the bounds on each case (e.g. see `arc.rs`).
Cheers,
Miguel
