Nouveau - Jul 2025 - [PATCH v13 2/5] rust: support formatting of foreign types

On Tue Jul 1, 2025 at 6:49 PM CEST, Tamir Duberstein
wrote:
> Introduce a `fmt!` macro which wraps all arguments in
> `kernel::fmt::Adapter` and a `kernel::fmt::Display` trait. This enables
> formatting of foreign types (like `core::ffi::CStr`) that do not
> implement `core::fmt::Display` due to concerns around lossy conversions
which
> do not apply in the kernel.
>
> Replace all direct calls to `format_args!` with `fmt!`.
>
> Replace all implementations of `core::fmt::Display` with implementations
> of `kernel::fmt::Display`.
>
> Suggested-by: Alice Ryhl <aliceryhl at google.com>
> Link:
https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467
> Acked-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> Reviewed-by: Alice Ryhl <aliceryhl at google.com>
> Signed-off-by: Tamir Duberstein <tamird at gmail.com>
> ---
>  drivers/block/rnull.rs       |  2 +-
>  drivers/gpu/nova-core/gpu.rs |  4 +-
>  rust/kernel/block/mq.rs      |  2 +-
>  rust/kernel/device.rs        |  2 +-
>  rust/kernel/fmt.rs           | 89 +++++++++++++++++++++++++++++++++++++++
>  rust/kernel/kunit.rs         |  6 +--
>  rust/kernel/lib.rs           |  1 +
>  rust/kernel/prelude.rs       |  3 +-
>  rust/kernel/print.rs         |  4 +-
>  rust/kernel/seq_file.rs      |  2 +-
>  rust/kernel/str.rs           | 22 ++++------
>  rust/macros/fmt.rs           | 99
++++++++++++++++++++++++++++++++++++++++++++
>  rust/macros/lib.rs           | 19 +++++++++
>  rust/macros/quote.rs         |  7 ++++
>  scripts/rustdoc_test_gen.rs  |  2 +-
>  15 files changed, 236 insertions(+), 28 deletions(-)
This would be a lot easier to review if he proc-macro and the call
replacement were different patches.

Also the `kernel/fmt.rs` file should be a different commit.
> diff --git a/rust/kernel/fmt.rs b/rust/kernel/fmt.rs
> new file mode 100644
> index 000000000000..348d16987de6
> --- /dev/null
> +++ b/rust/kernel/fmt.rs
> @@ -0,0 +1,89 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Formatting utilities.
> +
> +use core::fmt;
I think we should pub export all types that we are still using from
`core::fmt`. For example `Result`, `Formatter`, `Debug` etc.

That way I can still use the same pattern of importing `fmt` and then
writing

    impl fmt::Display for MyType {
        fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {}
    }
> +
> +/// Internal adapter used to route allow implementations of formatting
traits for foreign types.
> +///
> +/// It is inserted automatically by the [`fmt!`] macro and is not meant to
be used directly.
> +///
> +/// [`fmt!`]: crate::prelude::fmt!
> +#[doc(hidden)]
> +pub struct Adapter<T>(pub T);
> +
> +macro_rules! impl_fmt_adapter_forward {
> +    ($($trait:ident),* $(,)?) => {
> +        $(
> +            impl<T: fmt::$trait> fmt::$trait for Adapter<T> {
> +                fn fmt(&self, f: &mut
fmt::Formatter<'_>) -> fmt::Result {
> +                    let Self(t) = self;
> +                    fmt::$trait::fmt(t, f)
> +                }
> +            }
> +        )*
> +    };
> +}
> +
> +impl_fmt_adapter_forward!(Debug, LowerHex, UpperHex, Octal, Binary,
Pointer, LowerExp, UpperExp);
> +
> +/// A copy of [`fmt::Display`] that allows us to implement it for foreign
types.
> +///
> +/// Types should implement this trait rather than [`fmt::Display`].
Together with the [`Adapter`]
> +/// type and [`fmt!`] macro, it allows for formatting foreign types (e.g.
types from core) which do
> +/// not implement [`fmt::Display`] directly.
> +///
> +/// [`fmt!`]: crate::prelude::fmt!
> +pub trait Display {
> +    /// Same as [`fmt::Display::fmt`].
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) ->
fmt::Result;
> +}
> +
> +impl<T: ?Sized + Display> Display for &T {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) ->
fmt::Result {
> +        Display::fmt(*self, f)
> +    }
> +}
> +
> +impl<T: ?Sized + Display> fmt::Display for Adapter<&T> {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) ->
fmt::Result {
> +        let Self(t) = self;
> +        Display::fmt(t, f)
Why not `Display::fmt(&self.0, f)`?
> +    }
> +}
> +
> +macro_rules! impl_display_forward {
> +    ($(
> +        $( { $($generics:tt)* } )? $ty:ty $( { where $($where:tt)* } )?
> +    ),* $(,)?) => {
> +        $(
> +            impl$($($generics)*)? Display for $ty $(where $($where)*)? {
> +                fn fmt(&self, f: &mut
fmt::Formatter<'_>) -> fmt::Result {
> +                    fmt::Display::fmt(self, f)
> +                }
> +            }
> +        )*
> +    };
> +}
> +
> +impl_display_forward!(
> +    bool,
> +    char,
> +    core::panic::PanicInfo<'_>,
> +    fmt::Arguments<'_>,
> +    i128,
> +    i16,
> +    i32,
> +    i64,
> +    i8,
> +    isize,
> +    str,
> +    u128,
> +    u16,
> +    u32,
> +    u64,
> +    u8,
> +    usize,
> +    {<T: ?Sized>} crate::sync::Arc<T> {where
crate::sync::Arc<T>: fmt::Display},
> +    {<T: ?Sized>} crate::sync::UniqueArc<T> {where
crate::sync::UniqueArc<T>: fmt::Display},
> +);
> diff --git a/rust/macros/fmt.rs b/rust/macros/fmt.rs
> new file mode 100644
> index 000000000000..edc37c220a89
> --- /dev/null
> +++ b/rust/macros/fmt.rs
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use proc_macro::{Ident, TokenStream, TokenTree};
> +use std::collections::BTreeSet;
> +
> +/// Please see [`crate::fmt`] for documentation.
> +pub(crate) fn fmt(input: TokenStream) -> TokenStream {
> +    let mut input = input.into_iter();
> +
> +    let first_opt = input.next();
> +    let first_owned_str;
> +    let mut names = BTreeSet::new();
> +    let first_lit = {
> +        let Some((mut first_str, first_lit)) = (match first_opt.as_ref() {
> +            Some(TokenTree::Literal(first_lit)) => {
> +                first_owned_str = first_lit.to_string();
> +                Some(first_owned_str.as_str()).and_then(|first| {
> +                    let first = first.strip_prefix('"')?;
> +                    let first = first.strip_suffix('"')?;
> +                    Some((first, first_lit))
You're only using first_lit to get the span later, so why not just get
the span directly here?
> +                })
> +            }
> +            _ => None,
> +        }) else {
> +            return first_opt.into_iter().chain(input).collect();
> +        };
> +        while let Some((_, rest)) = first_str.split_once('{') {
Let's put a comment above this loop mentioning [1] and saying that it
parses the identifiers from the format arguments.

[1]: https://doc.rust-lang.org/std/fmt/index.html#syntax
> +            first_str = rest;
> +            if let Some(rest) = first_str.strip_prefix('{') {
> +                first_str = rest;
> +                continue;
> +            }
> +            if let Some((name, rest)) = first_str.split_once('}')
{
> +                first_str = rest;
> +                let name = name.split_once(':').map_or(name,
|(name, _)| name);
> +                if !name.is_empty() && !name.chars().all(|c|
c.is_ascii_digit()) {
> +                    names.insert(name);
> +                }
> +            }
> +        }
> +        first_lit
> +    };
> +
> +    let first_span = first_lit.span();
> +    let adapter = quote_spanned! {
> +        first_span => ::kernel::fmt::Adapter
> +    };
I think we should follow the formatting convention from the quote crate:

    let adapter = quote_spanned!(first_span=> ::kernel::fmt::Adapter);
> +
> +    let mut args = TokenStream::from_iter(first_opt);
> +    {
> +        let mut flush = |args: &mut TokenStream, current: &mut
TokenStream| {
You don't need to pass `args` as a closure argument, since you always
call it with `&mut args`.
> +            let current = std::mem::take(current);
> +            if !current.is_empty() {
> +                let (lhs, rhs) = (|| {
> +                    let mut current = current.into_iter();
> +                    let mut acc = TokenStream::new();
> +                    while let Some(tt) = current.next() {
> +                        // Split on `=` only once to handle cases like `a
= b = c`.
> +                        if matches!(&tt, TokenTree::Punct(p) if
p.as_char() == '=') {
> +                            names.remove(acc.to_string().as_str());
> +                            // Include the `=` itself to keep the handling
below uniform.
> +                            acc.extend([tt]);
> +                            return (Some(acc),
current.collect::<TokenStream>());
> +                        }
> +                        acc.extend([tt]);
> +                    }
> +                    (None, acc)
> +                })();
> +                args.extend(quote_spanned! {
> +                    first_span => #lhs #adapter(&#rhs)
> +                });
> +            }
> +        };
> +
> +        let mut current = TokenStream::new();
Define this before the closure, then you don't need to pass it as an
argument.

---
Cheers,
Benno
> +        for tt in input {
> +            match &tt {
> +                TokenTree::Punct(p) if p.as_char() == ',' => {
> +                    flush(&mut args, &mut current);
> +                    &mut args
> +                }
> +                _ => &mut current,
> +            }
> +            .extend([tt]);
> +        }
> +        flush(&mut args, &mut current);
> +    }
> +
> +    for name in names {
> +        let name = Ident::new(name, first_span);
> +        args.extend(quote_spanned! {
> +            first_span => , #name = #adapter(&#name)
> +        });
> +    }
> +
> +    quote_spanned! {
> +        first_span => ::core::format_args!(#args)
> +    }
> +}

On Thu, Jul 3, 2025 at 5:32?AM Benno Lossin <lossin at kernel.org>
wrote:
>
> On Tue Jul 1, 2025 at 6:49 PM CEST, Tamir Duberstein wrote:
> > Introduce a `fmt!` macro which wraps all arguments in
> > `kernel::fmt::Adapter` and a `kernel::fmt::Display` trait. This
enables
> > formatting of foreign types (like `core::ffi::CStr`) that do not
> > implement `core::fmt::Display` due to concerns around lossy
conversions which
> > do not apply in the kernel.
> >
> > Replace all direct calls to `format_args!` with `fmt!`.
> >
> > Replace all implementations of `core::fmt::Display` with
implementations
> > of `kernel::fmt::Display`.
> >
> > Suggested-by: Alice Ryhl <aliceryhl at google.com>
> > Link:
https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467
> > Acked-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> > Reviewed-by: Alice Ryhl <aliceryhl at google.com>
> > Signed-off-by: Tamir Duberstein <tamird at gmail.com>
> > ---
> >  drivers/block/rnull.rs       |  2 +-
> >  drivers/gpu/nova-core/gpu.rs |  4 +-
> >  rust/kernel/block/mq.rs      |  2 +-
> >  rust/kernel/device.rs        |  2 +-
> >  rust/kernel/fmt.rs           | 89
+++++++++++++++++++++++++++++++++++++++
> >  rust/kernel/kunit.rs         |  6 +--
> >  rust/kernel/lib.rs           |  1 +
> >  rust/kernel/prelude.rs       |  3 +-
> >  rust/kernel/print.rs         |  4 +-
> >  rust/kernel/seq_file.rs      |  2 +-
> >  rust/kernel/str.rs           | 22 ++++------
> >  rust/macros/fmt.rs           | 99
++++++++++++++++++++++++++++++++++++++++++++
> >  rust/macros/lib.rs           | 19 +++++++++
> >  rust/macros/quote.rs         |  7 ++++
> >  scripts/rustdoc_test_gen.rs  |  2 +-
> >  15 files changed, 236 insertions(+), 28 deletions(-)
>
> This would be a lot easier to review if he proc-macro and the call
> replacement were different patches.
>
> Also the `kernel/fmt.rs` file should be a different commit.
Can you help me understand why? The changes you ask to be separated
would all be in different files, so why would separate commits make it
easier to review?

I prefer to keep things in one commit because the changes are highly
interdependent. The proc macro doesn't make sense without
kernel/fmt.rs and kernel/fmt.rs is useless without the proc macro.
>
> > diff --git a/rust/kernel/fmt.rs b/rust/kernel/fmt.rs
> > new file mode 100644
> > index 000000000000..348d16987de6
> > --- /dev/null
> > +++ b/rust/kernel/fmt.rs
> > @@ -0,0 +1,89 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Formatting utilities.
> > +
> > +use core::fmt;
>
> I think we should pub export all types that we are still using from
> `core::fmt`. For example `Result`, `Formatter`, `Debug` etc.
>
> That way I can still use the same pattern of importing `fmt` and then
> writing
>
>     impl fmt::Display for MyType {
>         fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {}
>     }
Great idea, done for the next spin. It would be nice to be able to
lint against references to `core::fmt` outside of kernel/fmt.rs.
> > +
> > +/// Internal adapter used to route allow implementations of
formatting traits for foreign types.
> > +///
> > +/// It is inserted automatically by the [`fmt!`] macro and is not
meant to be used directly.
> > +///
> > +/// [`fmt!`]: crate::prelude::fmt!
> > +#[doc(hidden)]
> > +pub struct Adapter<T>(pub T);
> > +
> > +macro_rules! impl_fmt_adapter_forward {
> > +    ($($trait:ident),* $(,)?) => {
> > +        $(
> > +            impl<T: fmt::$trait> fmt::$trait for
Adapter<T> {
> > +                fn fmt(&self, f: &mut
fmt::Formatter<'_>) -> fmt::Result {
> > +                    let Self(t) = self;
> > +                    fmt::$trait::fmt(t, f)
> > +                }
> > +            }
> > +        )*
> > +    };
> > +}
> > +
> > +impl_fmt_adapter_forward!(Debug, LowerHex, UpperHex, Octal, Binary,
Pointer, LowerExp, UpperExp);
> > +
> > +/// A copy of [`fmt::Display`] that allows us to implement it for
foreign types.
> > +///
> > +/// Types should implement this trait rather than [`fmt::Display`].
Together with the [`Adapter`]
> > +/// type and [`fmt!`] macro, it allows for formatting foreign types
(e.g. types from core) which do
> > +/// not implement [`fmt::Display`] directly.
> > +///
> > +/// [`fmt!`]: crate::prelude::fmt!
> > +pub trait Display {
> > +    /// Same as [`fmt::Display::fmt`].
> > +    fn fmt(&self, f: &mut fmt::Formatter<'_>) ->
fmt::Result;
> > +}
> > +
> > +impl<T: ?Sized + Display> Display for &T {
> > +    fn fmt(&self, f: &mut fmt::Formatter<'_>) ->
fmt::Result {
> > +        Display::fmt(*self, f)
> > +    }
> > +}
> > +
> > +impl<T: ?Sized + Display> fmt::Display for
Adapter<&T> {
> > +    fn fmt(&self, f: &mut fmt::Formatter<'_>) ->
fmt::Result {
> > +        let Self(t) = self;
> > +        Display::fmt(t, f)
>
> Why not `Display::fmt(&self.0, f)`?
I like destructuring because it shows me that there's only one field.
With `self.0` I don't see that.
> > +    }
> > +}
> > +
> > +macro_rules! impl_display_forward {
> > +    ($(
> > +        $( { $($generics:tt)* } )? $ty:ty $( { where $($where:tt)* }
)?
> > +    ),* $(,)?) => {
> > +        $(
> > +            impl$($($generics)*)? Display for $ty $(where
$($where)*)? {
> > +                fn fmt(&self, f: &mut
fmt::Formatter<'_>) -> fmt::Result {
> > +                    fmt::Display::fmt(self, f)
> > +                }
> > +            }
> > +        )*
> > +    };
> > +}
> > +
> > +impl_display_forward!(
> > +    bool,
> > +    char,
> > +    core::panic::PanicInfo<'_>,
> > +    fmt::Arguments<'_>,
> > +    i128,
> > +    i16,
> > +    i32,
> > +    i64,
> > +    i8,
> > +    isize,
> > +    str,
> > +    u128,
> > +    u16,
> > +    u32,
> > +    u64,
> > +    u8,
> > +    usize,
> > +    {<T: ?Sized>} crate::sync::Arc<T> {where
crate::sync::Arc<T>: fmt::Display},
> > +    {<T: ?Sized>} crate::sync::UniqueArc<T> {where
crate::sync::UniqueArc<T>: fmt::Display},
> > +);
>
> > diff --git a/rust/macros/fmt.rs b/rust/macros/fmt.rs
> > new file mode 100644
> > index 000000000000..edc37c220a89
> > --- /dev/null
> > +++ b/rust/macros/fmt.rs
> > @@ -0,0 +1,99 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +use proc_macro::{Ident, TokenStream, TokenTree};
> > +use std::collections::BTreeSet;
> > +
> > +/// Please see [`crate::fmt`] for documentation.
> > +pub(crate) fn fmt(input: TokenStream) -> TokenStream {
> > +    let mut input = input.into_iter();
> > +
> > +    let first_opt = input.next();
> > +    let first_owned_str;
> > +    let mut names = BTreeSet::new();
> > +    let first_lit = {
> > +        let Some((mut first_str, first_lit)) = (match
first_opt.as_ref() {
> > +            Some(TokenTree::Literal(first_lit)) => {
> > +                first_owned_str = first_lit.to_string();
> > +                Some(first_owned_str.as_str()).and_then(|first| {
> > +                    let first =
first.strip_prefix('"')?;
> > +                    let first =
first.strip_suffix('"')?;
> > +                    Some((first, first_lit))
>
> You're only using first_lit to get the span later, so why not just get
> the span directly here?
Good point. I was probably using it for more stuff in an earlier iteration.
>
> > +                })
> > +            }
> > +            _ => None,
> > +        }) else {
> > +            return first_opt.into_iter().chain(input).collect();
> > +        };
> > +        while let Some((_, rest)) = first_str.split_once('{')
{
>
> Let's put a comment above this loop mentioning [1] and saying that it
> parses the identifiers from the format arguments.
>
> [1]: https://doc.rust-lang.org/std/fmt/index.html#syntax
?
>
> > +            first_str = rest;
> > +            if let Some(rest) = first_str.strip_prefix('{') {
> > +                first_str = rest;
> > +                continue;
> > +            }
> > +            if let Some((name, rest)) =
first_str.split_once('}') {
> > +                first_str = rest;
> > +                let name = name.split_once(':').map_or(name,
|(name, _)| name);
> > +                if !name.is_empty() && !name.chars().all(|c|
c.is_ascii_digit()) {
> > +                    names.insert(name);
> > +                }
> > +            }
> > +        }
> > +        first_lit
> > +    };
> > +
> > +    let first_span = first_lit.span();
> > +    let adapter = quote_spanned! {
> > +        first_span => ::kernel::fmt::Adapter
> > +    };
>
> I think we should follow the formatting convention from the quote crate:
>
>     let adapter = quote_spanned!(first_span=> ::kernel::fmt::Adapter);
Sure.
>
> > +
> > +    let mut args = TokenStream::from_iter(first_opt);
> > +    {
> > +        let mut flush = |args: &mut TokenStream, current:
&mut TokenStream| {
>
> You don't need to pass `args` as a closure argument, since you always
> call it with `&mut args`.
This doesn't work because of the borrow checker. If I wrote what you
suggest, then `args` is mutably borrowed by the closure, which
prohibits the mutable borrow needed for the .extend() call here:

        for tt in input {
            match &tt {
                TokenTree::Punct(p) if p.as_char() == ',' => {
                    flush(&mut args, &mut current);
                    &mut args
                }
                _ => &mut current,
            }
            .extend([tt]);
        }
>
> > +            let current = std::mem::take(current);
> > +            if !current.is_empty() {
> > +                let (lhs, rhs) = (|| {
> > +                    let mut current = current.into_iter();
> > +                    let mut acc = TokenStream::new();
> > +                    while let Some(tt) = current.next() {
> > +                        // Split on `=` only once to handle cases
like `a = b = c`.
> > +                        if matches!(&tt, TokenTree::Punct(p) if
p.as_char() == '=') {
> > +                            names.remove(acc.to_string().as_str());
> > +                            // Include the `=` itself to keep the
handling below uniform.
> > +                            acc.extend([tt]);
> > +                            return (Some(acc),
current.collect::<TokenStream>());
> > +                        }
> > +                        acc.extend([tt]);
> > +                    }
> > +                    (None, acc)
> > +                })();
> > +                args.extend(quote_spanned! {
> > +                    first_span => #lhs #adapter(&#rhs)
> > +                });
> > +            }
> > +        };
> > +
> > +        let mut current = TokenStream::new();
>
> Define this before the closure, then you don't need to pass it as an
> argument.
Same reason as above. Borrow checker says no.
>
> ---
> Cheers,
> Benno
>
> > +        for tt in input {
> > +            match &tt {
> > +                TokenTree::Punct(p) if p.as_char() == ','
=> {
> > +                    flush(&mut args, &mut current);
> > +                    &mut args
> > +                }
> > +                _ => &mut current,
> > +            }
> > +            .extend([tt]);
> > +        }
> > +        flush(&mut args, &mut current);
> > +    }
> > +
> > +    for name in names {
> > +        let name = Ident::new(name, first_span);
> > +        args.extend(quote_spanned! {
> > +            first_span => , #name = #adapter(&#name)
> > +        });
> > +    }
> > +
> > +    quote_spanned! {
> > +        first_span => ::core::format_args!(#args)
> > +    }
> > +}

On Thu Jul 3, 2025 at 3:55 PM CEST, Tamir Duberstein
wrote:
> On Thu, Jul 3, 2025 at 5:32?AM Benno Lossin <lossin at kernel.org>
wrote:
>> On Tue Jul 1, 2025 at 6:49 PM CEST, Tamir Duberstein wrote:
>> > Introduce a `fmt!` macro which wraps all arguments in
>> > `kernel::fmt::Adapter` and a `kernel::fmt::Display` trait. This
enables
>> > formatting of foreign types (like `core::ffi::CStr`) that do not
>> > implement `core::fmt::Display` due to concerns around lossy
conversions which
>> > do not apply in the kernel.
>> >
>> > Replace all direct calls to `format_args!` with `fmt!`.
>> >
>> > Replace all implementations of `core::fmt::Display` with
implementations
>> > of `kernel::fmt::Display`.
>> >
>> > Suggested-by: Alice Ryhl <aliceryhl at google.com>
>> > Link:
https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467
>> > Acked-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
>> > Reviewed-by: Alice Ryhl <aliceryhl at google.com>
>> > Signed-off-by: Tamir Duberstein <tamird at gmail.com>
>> > ---
>> >  drivers/block/rnull.rs       |  2 +-
>> >  drivers/gpu/nova-core/gpu.rs |  4 +-
>> >  rust/kernel/block/mq.rs      |  2 +-
>> >  rust/kernel/device.rs        |  2 +-
>> >  rust/kernel/fmt.rs           | 89
+++++++++++++++++++++++++++++++++++++++
>> >  rust/kernel/kunit.rs         |  6 +--
>> >  rust/kernel/lib.rs           |  1 +
>> >  rust/kernel/prelude.rs       |  3 +-
>> >  rust/kernel/print.rs         |  4 +-
>> >  rust/kernel/seq_file.rs      |  2 +-
>> >  rust/kernel/str.rs           | 22 ++++------
>> >  rust/macros/fmt.rs           | 99
++++++++++++++++++++++++++++++++++++++++++++
>> >  rust/macros/lib.rs           | 19 +++++++++
>> >  rust/macros/quote.rs         |  7 ++++
>> >  scripts/rustdoc_test_gen.rs  |  2 +-
>> >  15 files changed, 236 insertions(+), 28 deletions(-)
>>
>> This would be a lot easier to review if he proc-macro and the call
>> replacement were different patches.
>>
>> Also the `kernel/fmt.rs` file should be a different commit.
>
> Can you help me understand why? The changes you ask to be separated
> would all be in different files, so why would separate commits make it
> easier to review?
It takes less time to go through the entire patch and give a RB. I can
take smaller time chunks and don't have to get back into the entire
context of the patch when I don't have 30-60min available.

In this patch the biggest problem is the rename & addition of new
things, maybe just adding 200 lines in those files could be okay to go
together, see below for more.
> I prefer to keep things in one commit because the changes are highly
> interdependent. The proc macro doesn't make sense without
> kernel/fmt.rs and kernel/fmt.rs is useless without the proc macro.
I think that `Adapter`, the custom `Display` and their impl blocks
don't need to be in the same commit as the proc-macro. They are related,
but maybe someone is not well-versed in proc-macros and thus doesn't
want to review that part.
>> > diff --git a/rust/kernel/fmt.rs b/rust/kernel/fmt.rs
>> > new file mode 100644
>> > index 000000000000..348d16987de6
>> > --- /dev/null
>> > +++ b/rust/kernel/fmt.rs
>> > @@ -0,0 +1,89 @@
>> > +// SPDX-License-Identifier: GPL-2.0
>> > +
>> > +//! Formatting utilities.
>> > +
>> > +use core::fmt;
>>
>> I think we should pub export all types that we are still using from
>> `core::fmt`. For example `Result`, `Formatter`, `Debug` etc.
>>
>> That way I can still use the same pattern of importing `fmt` and then
>> writing
>>
>>     impl fmt::Display for MyType {
>>         fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result
{}
>>     }
>
> Great idea, done for the next spin. It would be nice to be able to
> lint against references to `core::fmt` outside of kernel/fmt.rs.
I think there was something in clippy that can do that globally and we
could allow that in this file?
>> > +
>> > +/// Internal adapter used to route allow implementations of
formatting traits for foreign types.
>> > +///
>> > +/// It is inserted automatically by the [`fmt!`] macro and is not
meant to be used directly.
>> > +///
>> > +/// [`fmt!`]: crate::prelude::fmt!
>> > +#[doc(hidden)]
>> > +pub struct Adapter<T>(pub T);
>> > +
>> > +macro_rules! impl_fmt_adapter_forward {
>> > +    ($($trait:ident),* $(,)?) => {
>> > +        $(
>> > +            impl<T: fmt::$trait> fmt::$trait for
Adapter<T> {
>> > +                fn fmt(&self, f: &mut
fmt::Formatter<'_>) -> fmt::Result {
>> > +                    let Self(t) = self;
>> > +                    fmt::$trait::fmt(t, f)
>> > +                }
>> > +            }
>> > +        )*
>> > +    };
>> > +}
>> > +
>> > +impl_fmt_adapter_forward!(Debug, LowerHex, UpperHex, Octal,
Binary, Pointer, LowerExp, UpperExp);
>> > +
>> > +/// A copy of [`fmt::Display`] that allows us to implement it for
foreign types.
>> > +///
>> > +/// Types should implement this trait rather than
[`fmt::Display`]. Together with the [`Adapter`]
>> > +/// type and [`fmt!`] macro, it allows for formatting foreign
types (e.g. types from core) which do
>> > +/// not implement [`fmt::Display`] directly.
>> > +///
>> > +/// [`fmt!`]: crate::prelude::fmt!
>> > +pub trait Display {
>> > +    /// Same as [`fmt::Display::fmt`].
>> > +    fn fmt(&self, f: &mut fmt::Formatter<'_>)
-> fmt::Result;
>> > +}
>> > +
>> > +impl<T: ?Sized + Display> Display for &T {
>> > +    fn fmt(&self, f: &mut fmt::Formatter<'_>)
-> fmt::Result {
>> > +        Display::fmt(*self, f)
>> > +    }
>> > +}
>> > +
>> > +impl<T: ?Sized + Display> fmt::Display for
Adapter<&T> {
>> > +    fn fmt(&self, f: &mut fmt::Formatter<'_>)
-> fmt::Result {
>> > +        let Self(t) = self;
>> > +        Display::fmt(t, f)
>>
>> Why not `Display::fmt(&self.0, f)`?
>
> I like destructuring because it shows me that there's only one field.
> With `self.0` I don't see that.
And what is the benefit here?
>> > +
>> > +    let mut args = TokenStream::from_iter(first_opt);
>> > +    {
>> > +        let mut flush = |args: &mut TokenStream, current:
&mut TokenStream| {
>>
>> You don't need to pass `args` as a closure argument, since you
always
>> call it with `&mut args`.
>
> This doesn't work because of the borrow checker. If I wrote what you
> suggest, then `args` is mutably borrowed by the closure, which
> prohibits the mutable borrow needed for the .extend() call here:
Ahh right... Well then it's fine.

---
Cheers,
Benno