[PATCH] rust: Implement fmt::Display for kernel Box<T, A>

[PATCH] rust: Implement fmt::Display for kernel Box<T, A>

[JCKeep]

Currently fmt::Display is not implemented for Box<T, A>. We can
make the following code work by implementing fmt::Display.

let v = KBox::new(42, GFP_KERNEL)?;
pr_info!("{}", v);

Link: https://github.com/Rust-for-Linux/linux/issues/1126
Signed-off-by: JCKeep <2407018371@qq.com>
---
 rust/kernel/alloc/kbox.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
index d69c32496..5f4765800 100644
--- a/rust/kernel/alloc/kbox.rs
+++ b/rust/kernel/alloc/kbox.rs
@@ -427,6 +427,16 @@ fn deref_mut(&mut self) -> &mut T {
     }
 }
 
+impl<T, A> fmt::Display for Box<T, A>
+where
+    T: ?Sized + fmt::Display,
+    A: Allocator,
+{
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        fmt::Display::fmt(&**self, f)
+    }
+}
+
 impl<T, A> fmt::Debug for Box<T, A>
 where
     T: ?Sized + fmt::Debug,
-- 
2.34.1

Re: [PATCH] rust: Implement fmt::Display for kernel Box<T, A>

[Tamir Duberstein]

On Mon, Oct 28, 2024 at 10:58 AM JCKeep <2407018371@qq.com> wrote:
>
> Currently fmt::Display is not implemented for Box<T, A>. We can
> make the following code work by implementing fmt::Display.
>
> let v = KBox::new(42, GFP_KERNEL)?;
> pr_info!("{}", v);
>
> Link: https://github.com/Rust-for-Linux/linux/issues/1126

Per the issue, I believe you are missing this tag:

: Suggested-by: Boqun Feng <boqun.feng@gmail.com>

> Signed-off-by: JCKeep <2407018371@qq.com>
> ---
>  rust/kernel/alloc/kbox.rs | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
> index d69c32496..5f4765800 100644
> --- a/rust/kernel/alloc/kbox.rs
> +++ b/rust/kernel/alloc/kbox.rs
> @@ -427,6 +427,16 @@ fn deref_mut(&mut self) -> &mut T {
>      }
>  }
>
> +impl<T, A> fmt::Display for Box<T, A>
> +where
> +    T: ?Sized + fmt::Display,
> +    A: Allocator,
> +{
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        fmt::Display::fmt(&**self, f)
> +    }
> +}
> +
>  impl<T, A> fmt::Debug for Box<T, A>
>  where
>      T: ?Sized + fmt::Debug,
> --
> 2.34.1
>
>

Reviewed-by: Tamir Duberstein <tamird@gmail.com>

Re: [PATCH] rust: Implement fmt::Display for kernel Box<T, A>

[Miguel Ojeda]

On Mon, Oct 28, 2024 at 3:58 PM JCKeep <2407018371@qq.com> wrote:
>
> Signed-off-by: JCKeep <2407018371@qq.com>

Thanks for the patch!

Is "JCKeep" a "known identity" (please see [1])?

[1] https://docs.kernel.org/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

Cheers,
Miguel

Re: [PATCH] rust: Implement fmt::Display for kernel Box<T, A>

[Boqun Feng]

Hi,

Thank you for the patch! A few comments below:

First, the patch title should be as tight as possible, please consider
using something like:

	rust: alloc: Implement Display for Box

* `Display` is a common trait in Rust, so can avoid mention its mod name
  (i.e. "fmt::").

* Since this is a *kernel* patch, so the word "kernel" can be avoided.

* The generic parameter is not related to the impl, so can be avoid.

On Mon, Oct 28, 2024 at 10:52:47PM +0800, JCKeep wrote:
> Currently fmt::Display is not implemented for Box<T, A>. We can
> make the following code work by implementing fmt::Display.
> 

Usually phrases like "We can" are not preferred in changelogs,
especially when describing the "why" part of the changes, because in
this part you want to describe the current situation based on *facts*
not on *desires*. So how about:

    Currently `impl Display` is missing for `Box<T, A>`, as a result,
    things like using `Box<..>` directly as an operand in `pr_info!()`
    are impossible, which is less ergonomic compared to `Box` in Rust
    std.

    Therefore add `impl Display` for `Box`.

?

> let v = KBox::new(42, GFP_KERNEL)?;
> pr_info!("{}", v);
> 
> Link: https://github.com/Rust-for-Linux/linux/issues/1126

Please use a Closes tag to refer an issue you want to close on GitHub.

Regards,
Boqun

> Signed-off-by: JCKeep <2407018371@qq.com>
> ---
>  rust/kernel/alloc/kbox.rs | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
> index d69c32496..5f4765800 100644
> --- a/rust/kernel/alloc/kbox.rs
> +++ b/rust/kernel/alloc/kbox.rs
> @@ -427,6 +427,16 @@ fn deref_mut(&mut self) -> &mut T {
>      }
>  }
>  
> +impl<T, A> fmt::Display for Box<T, A>
> +where
> +    T: ?Sized + fmt::Display,
> +    A: Allocator,
> +{
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        fmt::Display::fmt(&**self, f)
> +    }
> +}
> +
>  impl<T, A> fmt::Debug for Box<T, A>
>  where
>      T: ?Sized + fmt::Debug,
> -- 
> 2.34.1
> 

Re: [PATCH] rust: Implement fmt::Display for kernel Box<T, A>

[Miguel Ojeda]

On Mon, Oct 28, 2024 at 5:43 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Please use a Closes tag to refer an issue you want to close on GitHub.

Nit: it should be Link, because Closes is restricted to bug fixes
currently, according to the docs (well, at least my reading of them).

Cheers,
Miguel

Re: [PATCH] rust: Implement fmt::Display for kernel Box<T, A>

[Boqun Feng]

On Mon, Oct 28, 2024 at 05:52:54PM +0100, Miguel Ojeda wrote:
> On Mon, Oct 28, 2024 at 5:43 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > Please use a Closes tag to refer an issue you want to close on GitHub.
> 
> Nit: it should be Link, because Closes is restricted to bug fixes
> currently, according to the docs (well, at least my reading of them).
> 

Then I guess I'm using it wrongly ;-) Thanks!

Regards,
Boqun

> Cheers,
> Miguel