From: Jimmy Ostler <jtostler1@gmail.com>

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@gmail.com>
Suggested-by: Boqun Feng <boqun.feng@gmail.com>
Link: https://github.com/Rust-for-Linux/linux/issues/1131
---
 rust/kernel/alloc/layout.rs | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/rust/kernel/alloc/layout.rs b/rust/kernel/alloc/layout.rs
index 4b3cd7fdc816..4265f92f8af0 100644
--- a/rust/kernel/alloc/layout.rs
+++ b/rust/kernel/alloc/layout.rs
@@ -7,6 +7,7 @@
 use core::{alloc::Layout, marker::PhantomData};
 
 /// Error when constructing an [`ArrayLayout`].
+#[derive(Debug)]
 pub struct LayoutError;
 
 /// A layout for an array `[T; n]`.
@@ -43,6 +44,19 @@ pub const fn empty() -> Self {
     /// # Errors
     ///
     /// When `len * size_of::<T>()` overflows or when `len * size_of::<T>() > isize::MAX`.
+    ///
+    ///
+    /// # Examples
+    ///
+    /// ```rust
+    /// use kernel::alloc::layout::ArrayLayout;
+    ///
+    /// let layout = ArrayLayout::<i32>::new(15);
+    /// assert_eq!(layout.expect("len * size_of::<i32>() does not overflow").len(), 15);
+    ///
+    /// let layout = ArrayLayout::<i32>::new(isize::MAX as usize);
+    /// assert!(layout.is_err());
+    /// ```
     pub const fn new(len: usize) -> Result<Self, LayoutError> {
         match len.checked_mul(core::mem::size_of::<T>()) {
             Some(size) if size <= ISIZE_MAX => {

base-commit: 1dc707e647bc919834eff9636c8d00b78c782545
-- 
2.47.1

On Tue, Dec 3, 2024 at 6:19 AM <jtostler1@gmail.com> wrote:
>
> From: Jimmy Ostler <jtostler1@gmail.com>
>
> 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@gmail.com>
> Suggested-by: Boqun Feng <boqun.feng@gmail.com>
> 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

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

On Wed, Dec 4, 2024 at 2:57 AM <jtostler1@gmail.com> wrote:
>
> 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?
>
> Same as above about v2 patch messages. Should I add it to commit, or
> include that in patch changelogs?

The changelog between series goes outside the commit, and is meant to
help reviewers see what you changed (otherwise they would need to run
e.g. a range diff and visually inspect it). The commits themselves
should describe the changes in that commit, ignoring what happened in
other versions, and it is meant for the future to explain what the
change is and why it was introduced in the tree.

So if you do A in v1 and B in v2, then the commit message of v1 would
explain A, v2 would explain B, and the changelog v1->v2 for reviewers
simply states the A->B change (i.e. briefly) so that reviewers know
what happened.

[ Sometimes it is important to explain in the commit message
nevertheless why an approach like B is better than A, but you would do
that in order to clarify why B was picked, not just because A happened
to appear in a previous version because it was tried first and
discarded. ]

I hope that makes sense!

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

Great, thanks!

Cheers,
Miguel

On Tue, Dec 3, 2024 at 6:19 AM <jtostler1@gmail.com> wrote:
>
> From: Jimmy Ostler <jtostler1@gmail.com>
>
> 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@gmail.com>
> Suggested-by: Boqun Feng <boqun.feng@gmail.com>
> Link: https://github.com/Rust-for-Linux/linux/issues/1131
> ---
>  rust/kernel/alloc/layout.rs | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/rust/kernel/alloc/layout.rs b/rust/kernel/alloc/layout.rs
> index 4b3cd7fdc816..4265f92f8af0 100644
> --- a/rust/kernel/alloc/layout.rs
> +++ b/rust/kernel/alloc/layout.rs
> @@ -7,6 +7,7 @@
>  use core::{alloc::Layout, marker::PhantomData};
>
>  /// Error when constructing an [`ArrayLayout`].
> +#[derive(Debug)]
>  pub struct LayoutError;
>
>  /// A layout for an array `[T; n]`.
> @@ -43,6 +44,19 @@ pub const fn empty() -> Self {
>      /// # Errors
>      ///
>      /// When `len * size_of::<T>()` overflows or when `len * size_of::<T>() > isize::MAX`.
> +    ///
> +    ///
> +    /// # Examples

There should only be one empty line before # Examples, not two.

> +    ///
> +    /// ```rust
> +    /// use kernel::alloc::layout::ArrayLayout;
> +    ///
> +    /// let layout = ArrayLayout::<i32>::new(15);
> +    /// assert_eq!(layout.len(), 15);

I think it's less confusing to move this expect() to the line before.
let layout = ArrayLayout::<i32>::new(15).expect(...);

> +    ///
> +    /// let layout = ArrayLayout::<i32>::new(isize::MAX as usize);
> +    /// assert!(layout.is_err());
> +    /// ```
>      pub const fn new(len: usize) -> Result<Self, LayoutError> {
>          match len.checked_mul(core::mem::size_of::<T>()) {
>              Some(size) if size <= ISIZE_MAX => {
>
> base-commit: 1dc707e647bc919834eff9636c8d00b78c782545
> --
> 2.47.1
>
>