Re: [PATCH] rust: alloc: Add doctest for `ArrayLayout`

[Miguel Ojeda]

On Tue, Dec 3, 2024 at 6:19 AM <jtostler1@xxxxxxxxx> wrote:
>
> From: Jimmy Ostler <jtostler1@xxxxxxxxx>
>
> Added a rustdoc example and Kunit test to the `ArrayLayout` struct's
> `ArrayLayout::new()` function.
>
> Kunit tests ran using `./tools/testing/kunit/kunit.py run \
> --make_options LLVM=1 \
> --kconfig_add CONFIG_RUST=y` passed.
>
> Generated documentation looked as expected.
>
> Signed-off-by: Jimmy Ostler <jtostler1@xxxxxxxxx>
> Suggested-by: Boqun Feng <boqun.feng@xxxxxxxxx>
> Link: https://github.com/Rust-for-Linux/linux/issues/1131

Thanks for the patch!

A few procedural nits: please Cc the maintainers/reviewers, especially
the main one (Danilo) -- for that, please see
`scripts/get_maintainer.pl` as well as e.g.
https://rust-for-linux.com/contributing#submitting-patches for one way
to generate the arguments.

The "Signed-off-by" tag normally would be the last one -- that way
people see that you added the other two rather than the next person in
the chain. It is good to mention the tests etc. that you have done,
although normally for a patch like this it would normally not be
mentioned (since all patches that add an example need to be tested
anyway).

Finally, a nit on the commit message: normally they are written in the
imperative mood.

By the way, the "From:" tag on the top would not need to be there if
your "From:" in the email headers is configured properly.

>  /// Error when constructing an [`ArrayLayout`].
> +#[derive(Debug)]
>  pub struct LayoutError;

Ideally you would mention this change in the commit message too -- it
is the only non-comment/doc change, after all :) It is also important
because, in general, so far, we have not been using `expect`.

> +    ///
> +    ///

Please use a single line.

> +    /// ```rust

You can remove "rust" since it is the default.

> +    /// use kernel::alloc::layout::ArrayLayout;

This line could be hidden -- it is `Self`, after all, so it is not
adding much for the reader. We are not fully consistent on this yet
though.

> +    /// let layout = ArrayLayout::<i32>::new(15);
> +    /// assert_eq!(layout.expect("len * size_of::<i32>() does not overflow").len(), 15);

See above on `expect`.

Moreover, since it is a test, it is fine to panic, but recently we
were discussing that examples should ideally show how "real code"
would be written, thus using `?` etc. instead.

> +    /// let layout = ArrayLayout::<i32>::new(isize::MAX as usize);

Perhaps we could consider an example with an smaller argument that
still overflows, to drive home the multiplication involved. It could
perhaps be a third one.

Thanks!

Cheers,
Miguel