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

[jtostler1@gmail.com]

On Tue, Dec 3, 2024 at 12:48 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> Thanks for the patch!

You're welcome! Thank you once again for creating these opportunities!

> 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.

Thanks for the pointer, I've got that fixed up now.

> 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.

Looking at the documentation for sending patches it's unclear whether
the commit message for a v2 of a patch should be modified for cases 
like this, or the only changes should be the in patch changelogs,
after the marker line. What would usually be the preferred course of
action in cases like this?

> >  /// 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`.

Same as above about v2 patch messages. Should I add it to commit, or 
include that in patch changelogs?

> > +    ///
> > +    ///
>
> Please use a single line.
>
> > +    /// ```rust
>
> You can remove "rust" since it is the default.

Fixed both of these.

> > +    /// 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.

Done.

> > +    /// 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.

Changing it to use `?`.

>
> > +    /// 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.

I agree, I think adding a third where the length is set to
`isize::MAX as usize / 2` illustrates how when `len < isize::MAX`,
the overflow can still occur.

>
> Thanks!
>
> Cheers,
> Miguel

Thanks,
Jimmy