rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] rust: Implement fmt::Display for kernel Box<T, A>
@ 2024-10-28 14:52 JCKeep
  2024-10-28 15:03 ` Tamir Duberstein
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: JCKeep @ 2024-10-28 14:52 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor
  Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
	Andreas Hindborg, Alice Ryhl, Trevor Gross, rust-for-linux,
	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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] rust: Implement fmt::Display for kernel Box<T, A>
  2024-10-28 14:52 [PATCH] rust: Implement fmt::Display for kernel Box<T, A> JCKeep
@ 2024-10-28 15:03 ` Tamir Duberstein
  2024-10-28 15:58 ` Miguel Ojeda
  2024-10-28 16:43 ` Boqun Feng
  2 siblings, 0 replies; 6+ messages in thread
From: Tamir Duberstein @ 2024-10-28 15:03 UTC (permalink / raw)
  To: JCKeep
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, rust-for-linux

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>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rust: Implement fmt::Display for kernel Box<T, A>
  2024-10-28 14:52 [PATCH] rust: Implement fmt::Display for kernel Box<T, A> JCKeep
  2024-10-28 15:03 ` Tamir Duberstein
@ 2024-10-28 15:58 ` Miguel Ojeda
  2024-10-28 16:43 ` Boqun Feng
  2 siblings, 0 replies; 6+ messages in thread
From: Miguel Ojeda @ 2024-10-28 15:58 UTC (permalink / raw)
  To: JCKeep
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, rust-for-linux

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rust: Implement fmt::Display for kernel Box<T, A>
  2024-10-28 14:52 [PATCH] rust: Implement fmt::Display for kernel Box<T, A> JCKeep
  2024-10-28 15:03 ` Tamir Duberstein
  2024-10-28 15:58 ` Miguel Ojeda
@ 2024-10-28 16:43 ` Boqun Feng
  2024-10-28 16:52   ` Miguel Ojeda
  2 siblings, 1 reply; 6+ messages in thread
From: Boqun Feng @ 2024-10-28 16:43 UTC (permalink / raw)
  To: JCKeep
  Cc: Miguel Ojeda, Alex Gaynor, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	rust-for-linux

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
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rust: Implement fmt::Display for kernel Box<T, A>
  2024-10-28 16:43 ` Boqun Feng
@ 2024-10-28 16:52   ` Miguel Ojeda
  2024-10-28 17:39     ` Boqun Feng
  0 siblings, 1 reply; 6+ messages in thread
From: Miguel Ojeda @ 2024-10-28 16:52 UTC (permalink / raw)
  To: Boqun Feng
  Cc: JCKeep, Miguel Ojeda, Alex Gaynor, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	rust-for-linux

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rust: Implement fmt::Display for kernel Box<T, A>
  2024-10-28 16:52   ` Miguel Ojeda
@ 2024-10-28 17:39     ` Boqun Feng
  0 siblings, 0 replies; 6+ messages in thread
From: Boqun Feng @ 2024-10-28 17:39 UTC (permalink / raw)
  To: Miguel Ojeda
  Cc: JCKeep, Miguel Ojeda, Alex Gaynor, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	rust-for-linux

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-10-28 17:39 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-28 14:52 [PATCH] rust: Implement fmt::Display for kernel Box<T, A> JCKeep
2024-10-28 15:03 ` Tamir Duberstein
2024-10-28 15:58 ` Miguel Ojeda
2024-10-28 16:43 ` Boqun Feng
2024-10-28 16:52   ` Miguel Ojeda
2024-10-28 17:39     ` Boqun Feng

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).