Re: [PATCH v3] rust: add bindings and API for bitmap.h and bitops.h.

[Burak Emir]

On Mon, Mar 10, 2025 at 5:53 PM Miguel Ojeda
<miguel.ojeda.sandonis@xxxxxxxxx> wrote:
>
> Hi Burak,
>
> Some quick notes...
>
> On Mon, Mar 10, 2025 at 5:20 PM Burak Emir <bqe@xxxxxxxxxx> wrote:
> >
> > +void rust_helper_bitmap_copy_and_extend(unsigned long *dst, const unsigned long *src, unsigned int count, unsigned int size)
> > +{
> > +       bitmap_copy_and_extend(dst, src, count, size);
> > +}
>
> Please use the same parameter names as the real one, i.e. `to` and `from`.
>
> > +/// Wraps underlying C bitmap structure.
>
> I am not sure I would say it "structure" here, i.e. it seems like it
> actually wraps a C `struct bitmap`.
>
> In general, we also try to mention the "wraps ..." (if it actually did
> so) in a second paragraph, rather than doing so in the title.
>
> > +/// # Invariants
> > +///
> > +/// * `ptr` is obtained from a successful call to `bitmap_zalloc` and
>
> Also, you may remove the bullet list -- we currently do not enforce
> that it is always a bullet list (though we may in the future).
>
> > +    /// Pointer to an array of unsigned long.
>
> `unsigned long`
>
> > +    ptr: NonNull<usize>,
> > +    /// How many bits this bitmap stores. Must be < 2^32.
>
> Should the "Must be" be part of the invariants above?
>
> > +        // SAFETY: `self.ptr` was returned by bitmap_zalloc.
>
> "the C `bitmap_zalloc`"
>
> > +impl Bitmap {
> > +    /// Constructs a new [`Bitmap`].
> > +    ///
> > +    /// Fails with AllocError if `nbits` is greater than or equal to 2^32,
>
> Intra-doc links where possible: [`AllocError`].
>
> > +    /// # Example
>
> We use plurals even if there is a single example (like for the invariants).
>
> > +    /// ```
> > +    /// # use kernel::bitmap::Bitmap;
> > +    ///
> > +    /// fn new_bitmap() -> Bitmap {
> > +    ///   Bitmap::new(128, GFP_KERNEL).unwrap()
> > +    /// }
> > +    /// ```
>
> Is there a reason why this example cannot be run? i.e. to not have it
> wrapped in a function.
>
> Also, please use the `?` operator if possible -- we try to write
> examples as "real code", so we try to avoid `unwrap()` etc.
>
> > +            Ok(Bitmap { ptr, nbits })
>
> When we have invariants, we use an `// INVARIANT: ...` comment (please
> grep for similar cases to see how it is usually done).
>
> > +    /// Returns how many bits this bitmap holds.
>
> We should have examples (which double as tests) for these.
>
> One alternative, that may be simpler to showcase how the type works,
> is to write a longer example in the documentation of the type itself.
>
> > +        // SAFETY: nbits == 0 is supported and access is within bounds.
>
> Markdown wherever possible, e.g. `nbits == 0` (also other instance).
>
> Thanks!
>

Thanks, all done. Paying more attention to the markdown now and adding
a longer example.

Cheers,
Burak

> Cheers,
> Miguel