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@google.com>
Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Tamir Duberstein <tamird@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(-)
diff --git a/drivers/block/rnull.rs b/drivers/block/rnull.rs
index d07e76ae2c13..6366da12c5a5 100644
--- a/drivers/block/rnull.rs
+++ b/drivers/block/rnull.rs
@@ -51,7 +51,7 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
.logical_block_size(4096)?
.physical_block_size(4096)?
.rotational(false)
- .build(format_args!("rnullb{}", 0), tagset)
+ .build(fmt!("rnullb{}", 0), tagset)
})();
try_pin_init!(Self {
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 60b86f370284..e6b208175ff9 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -91,7 +91,7 @@ pub(crate) fn arch(&self) -> Architecture {
// Hence, replace with something like strum_macros derive(Display).
//
// For now, redirect to fmt::Debug for convenience.
-impl fmt::Display for Chipset {
+impl kernel::fmt::Display for Chipset {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{self:?}")
}
@@ -132,7 +132,7 @@ fn from_boot0(boot0: regs::NV_PMC_BOOT_0) -> Self {
}
}
-impl fmt::Display for Revision {
+impl kernel::fmt::Display for Revision {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{:x}.{:x}", self.major, self.minor)
}
diff --git a/rust/kernel/block/mq.rs b/rust/kernel/block/mq.rs
index 831445d37181..61ea35bba7d5 100644
--- a/rust/kernel/block/mq.rs
+++ b/rust/kernel/block/mq.rs
@@ -82,7 +82,7 @@
//! Arc::pin_init(TagSet::new(1, 256, 1), flags::GFP_KERNEL)?;
//! let mut disk = gen_disk::GenDiskBuilder::new()
//! .capacity_sectors(4096)
-//! .build(format_args!("myblk"), tagset)?;
+//! .build(fmt!("myblk"), tagset)?;
//!
//! # Ok::<(), kernel::error::Error>(())
//! ```
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 5c946af3a4d5..86b945576780 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -345,7 +345,7 @@ macro_rules! impl_device_context_into_aref {
macro_rules! dev_printk {
($method:ident, $dev:expr, $($f:tt)*) => {
{
- ($dev).$method(::core::format_args!($($f)*));
+ ($dev).$method($crate::prelude::fmt!($($f)*));
}
}
}
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;
+
+/// 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)
+ }
+}
+
+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/kernel/kunit.rs b/rust/kernel/kunit.rs
index 099a61bbb8f4..3539edbceaf5 100644
--- a/rust/kernel/kunit.rs
+++ b/rust/kernel/kunit.rs
@@ -74,14 +74,14 @@ macro_rules! kunit_assert {
// mistake (it is hidden to prevent that).
//
// This mimics KUnit's failed assertion format.
- $crate::kunit::err(format_args!(
+ $crate::kunit::err($crate::prelude::fmt!(
" # {}: ASSERTION FAILED at {FILE}:{LINE}\n",
$name
));
- $crate::kunit::err(format_args!(
+ $crate::kunit::err($crate::prelude::fmt!(
" Expected {CONDITION} to be true, but is false\n"
));
- $crate::kunit::err(format_args!(
+ $crate::kunit::err($crate::prelude::fmt!(
" Failure not reported to KUnit since this is a non-KUnit task\n"
));
break 'out;
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 6b4774b2b1c3..aadcfaa5c759 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -77,6 +77,7 @@
pub mod faux;
#[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
pub mod firmware;
+pub mod fmt;
pub mod fs;
pub mod init;
pub mod io;
diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
index 2f30a398dddd..41cebd906c4c 100644
--- a/rust/kernel/prelude.rs
+++ b/rust/kernel/prelude.rs
@@ -22,7 +22,7 @@
pub use crate::alloc::{flags::*, Box, KBox, KVBox, KVVec, KVec, VBox, VVec, Vec};
#[doc(no_inline)]
-pub use macros::{export, kunit_tests, module, vtable};
+pub use macros::{export, fmt, kunit_tests, module, vtable};
pub use pin_init::{init, pin_data, pin_init, pinned_drop, InPlaceWrite, Init, PinInit, Zeroable};
@@ -31,7 +31,6 @@
// `super::std_vendor` is hidden, which makes the macro inline for some reason.
#[doc(no_inline)]
pub use super::dbg;
-pub use super::fmt;
pub use super::{dev_alert, dev_crit, dev_dbg, dev_emerg, dev_err, dev_info, dev_notice, dev_warn};
pub use super::{pr_alert, pr_crit, pr_debug, pr_emerg, pr_err, pr_info, pr_notice, pr_warn};
diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs
index ecdcee43e5a5..d9c619edcd2f 100644
--- a/rust/kernel/print.rs
+++ b/rust/kernel/print.rs
@@ -149,7 +149,7 @@ macro_rules! print_macro (
// takes borrows on the arguments, but does not extend the scope of temporaries.
// Therefore, a `match` expression is used to keep them around, since
// the scrutinee is kept until the end of the `match`.
- match format_args!($($arg)+) {
+ match $crate::prelude::fmt!($($arg)+) {
// SAFETY: This hidden macro should only be called by the documented
// printing macros which ensure the format string is one of the fixed
// ones. All `__LOG_PREFIX`s are null-terminated as they are generated
@@ -168,7 +168,7 @@ macro_rules! print_macro (
// The `CONT` case.
($format_string:path, true, $($arg:tt)+) => (
$crate::print::call_printk_cont(
- format_args!($($arg)+),
+ $crate::prelude::fmt!($($arg)+),
);
);
);
diff --git a/rust/kernel/seq_file.rs b/rust/kernel/seq_file.rs
index 8f199b1a3bb1..05c9b7bec727 100644
--- a/rust/kernel/seq_file.rs
+++ b/rust/kernel/seq_file.rs
@@ -47,7 +47,7 @@ pub fn call_printf(&self, args: core::fmt::Arguments<'_>) {
#[macro_export]
macro_rules! seq_print {
($m:expr, $($arg:tt)+) => (
- $m.call_printf(format_args!($($arg)+))
+ $m.call_printf($crate::prelude::fmt!($($arg)+))
);
}
pub use seq_print;
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index cbc8b459ed41..d3fa4b703d35 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -50,11 +50,11 @@ pub fn strip_prefix(&self, pattern: impl AsRef<Self>) -> Option<&BStr> {
}
}
-impl fmt::Display for BStr {
+impl crate::fmt::Display for BStr {
/// Formats printable ASCII characters, escaping the rest.
///
/// ```
- /// # use kernel::{fmt, b_str, str::{BStr, CString}};
+ /// # use kernel::{prelude::fmt, b_str, str::{BStr, CString}};
/// let ascii = b_str!("Hello, BStr!");
/// let s = CString::try_from_fmt(fmt!("{}", ascii))?;
/// assert_eq!(s.as_bytes(), "Hello, BStr!".as_bytes());
@@ -85,7 +85,7 @@ impl fmt::Debug for BStr {
/// escaping the rest.
///
/// ```
- /// # use kernel::{fmt, b_str, str::{BStr, CString}};
+ /// # use kernel::{prelude::fmt, b_str, str::{BStr, CString}};
/// // Embedded double quotes are escaped.
/// let ascii = b_str!("Hello, \"BStr\"!");
/// let s = CString::try_from_fmt(fmt!("{:?}", ascii))?;
@@ -424,12 +424,12 @@ pub fn to_ascii_uppercase(&self) -> Result<CString, AllocError> {
}
}
-impl fmt::Display for CStr {
+impl crate::fmt::Display for CStr {
/// Formats printable ASCII characters, escaping the rest.
///
/// ```
/// # use kernel::c_str;
- /// # use kernel::fmt;
+ /// # use kernel::prelude::fmt;
/// # use kernel::str::CStr;
/// # use kernel::str::CString;
/// let penguin = c_str!("🐧");
@@ -459,7 +459,7 @@ impl fmt::Debug for CStr {
///
/// ```
/// # use kernel::c_str;
- /// # use kernel::fmt;
+ /// # use kernel::prelude::fmt;
/// # use kernel::str::CStr;
/// # use kernel::str::CString;
/// let penguin = c_str!("🐧");
@@ -578,7 +578,7 @@ mod tests {
macro_rules! format {
($($f:tt)*) => ({
- CString::try_from_fmt(::kernel::fmt!($($f)*))?.to_str()?
+ CString::try_from_fmt(crate::prelude::fmt!($($f)*))?.to_str()?
})
}
@@ -840,7 +840,7 @@ fn write_str(&mut self, s: &str) -> fmt::Result {
/// # Examples
///
/// ```
-/// use kernel::{str::CString, fmt};
+/// use kernel::{str::CString, prelude::fmt};
///
/// let s = CString::try_from_fmt(fmt!("{}{}{}", "abc", 10, 20))?;
/// assert_eq!(s.as_bytes_with_nul(), "abc1020\0".as_bytes());
@@ -930,9 +930,3 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt::Debug::fmt(&**self, f)
}
}
-
-/// A convenience alias for [`core::format_args`].
-#[macro_export]
-macro_rules! fmt {
- ($($f:tt)*) => ( ::core::format_args!($($f)*) )
-}
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))
+ })
+ }
+ _ => None,
+ }) else {
+ return first_opt.into_iter().chain(input).collect();
+ };
+ while let Some((_, rest)) = first_str.split_once('{') {
+ 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
+ };
+
+ let mut args = TokenStream::from_iter(first_opt);
+ {
+ let mut flush = |args: &mut TokenStream, current: &mut TokenStream| {
+ 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();
+ 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)
+ }
+}
diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
index fa847cf3a9b5..980e70d253e4 100644
--- a/rust/macros/lib.rs
+++ b/rust/macros/lib.rs
@@ -15,6 +15,7 @@
mod quote;
mod concat_idents;
mod export;
+mod fmt;
mod helpers;
mod kunit;
mod module;
@@ -201,6 +202,24 @@ pub fn export(attr: TokenStream, ts: TokenStream) -> TokenStream {
export::export(attr, ts)
}
+/// Like [`core::format_args!`], but automatically wraps arguments in [`kernel::fmt::Adapter`].
+///
+/// This macro allows generating `core::fmt::Arguments` while ensuring that each argument is wrapped
+/// with `::kernel::fmt::Adapter`, which customizes formatting behavior for kernel logging.
+///
+/// Named arguments used in the format string (e.g. `{foo}`) are detected and resolved from local
+/// bindings. All positional and named arguments are automatically wrapped.
+///
+/// This macro is an implementation detail of other kernel logging macros like [`pr_info!`] and
+/// should not typically be used directly.
+///
+/// [`kernel::fmt::Adapter`]: ../kernel/fmt/struct.Adapter.html
+/// [`pr_info!`]: ../kernel/macro.pr_info.html
+#[proc_macro]
+pub fn fmt(input: TokenStream) -> TokenStream {
+ fmt::fmt(input)
+}
+
/// Concatenate two identifiers.
///
/// This is useful in macros that need to declare or reference items with names
diff --git a/rust/macros/quote.rs b/rust/macros/quote.rs
index acc140c18653..ddfc21577539 100644
--- a/rust/macros/quote.rs
+++ b/rust/macros/quote.rs
@@ -48,6 +48,7 @@ macro_rules! quote_spanned {
($span:expr => $($tt:tt)*) => {{
let mut tokens = ::proc_macro::TokenStream::new();
{
+ #[allow(unused_variables)]
let span = $span;
quote_spanned!(@proc tokens span $($tt)*);
}
@@ -146,6 +147,12 @@ macro_rules! quote_spanned {
)]);
quote_spanned!(@proc $v $span $($tt)*);
};
+ (@proc $v:ident $span:ident & $($tt:tt)*) => {
+ $v.extend([::proc_macro::TokenTree::Punct(
+ ::proc_macro::Punct::new('&', ::proc_macro::Spacing::Alone),
+ )]);
+ quote_spanned!(@proc $v $span $($tt)*);
+ };
(@proc $v:ident $span:ident _ $($tt:tt)*) => {
$v.extend([::proc_macro::TokenTree::Ident(
::proc_macro::Ident::new("_", $span),
diff --git a/scripts/rustdoc_test_gen.rs b/scripts/rustdoc_test_gen.rs
index 1ca253594d38..507d36875196 100644
--- a/scripts/rustdoc_test_gen.rs
+++ b/scripts/rustdoc_test_gen.rs
@@ -201,7 +201,7 @@ macro_rules! assert_eq {{
// This follows the syntax for declaring test metadata in the proposed KTAP v2 spec, which may
// be used for the proposed KUnit test attributes API. Thus hopefully this will make migration
// easier later on.
- ::kernel::kunit::info(format_args!(" # {kunit_name}.location: {real_path}:{line}\n"));
+ ::kernel::kunit::info(fmt!(" # {kunit_name}.location: {real_path}:{line}\n"));
/// The anchor where the test code body starts.
#[allow(unused)]
--
2.50.0
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@google.com>
> Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Tamir Duberstein <tamird@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@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@google.com>
> > Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467
> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Tamir Duberstein <tamird@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:56 PM Tamir Duberstein <tamird@gmail.com> wrote: > > 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? By the way, if we are talking about splitting, it is easier to land patches that can go independently into different subsystems and avoiding flag day changes (or making those as small as possible), i.e. ideally being able to land big changes across more than one kernel cycle. Cheers, Miguel
On Thu, Jul 3, 2025 at 12:26 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Thu, Jul 3, 2025 at 3:56 PM Tamir Duberstein <tamird@gmail.com> wrote: > > > > 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? > > By the way, if we are talking about splitting, it is easier to land > patches that can go independently into different subsystems and > avoiding flag day changes (or making those as small as possible), i.e. > ideally being able to land big changes across more than one kernel > cycle. Understood, though in this case I don't see how it's workable. The formatting macros can either wrap in fmt::Adapter (and thus require kernel::fmt::Display) or not (and thus require core::fmt::Display), but I don't see how they can work in a mixed world. We can't have half the subsystems implement core::fmt::Display and the other half implement kernel::fmt::Display.
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@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@google.com>
>> > Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467
>> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> > Signed-off-by: Tamir Duberstein <tamird@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
On Thu, Jul 3, 2025 at 11:08 AM Benno Lossin <lossin@kernel.org> wrote:
>
> 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@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@google.com>
> >> > Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467
> >> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> >> > Signed-off-by: Tamir Duberstein <tamird@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.
Ah, I see what you mean. Yeah, the requirement to RB the entire patch
does mean there's a benefit to smaller patches.
> 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.
After implementing your suggestion of re-exporting things from
`kernel::fmt` the diffstat is
26 files changed, 253 insertions(+), 51 deletions(-)
so I guess I could do all the additions in one patch, but then
*everything* else has to go in a single patch together because the
formatting macros either want core::fmt::Display or
kernel::fmt::Display; they can't work in a halfway state.
>
> > 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.
Sure, I guess I will split them. But as noted above: changing the
formatting macros and all the types' trait implementations has to be a
"flag day" change.
>
> >> > 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?
I didn't find anything suitable. Do you have one in mind?
> >> > +
> >> > +/// 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?
In general the benefit is that the method does not ignore some portion
of `Self`. A method that uses `self.0` would not provoke a compiler
error in case another field is added, while this form would.
>
> >> > +
> >> > + 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
On Thu, Jul 03, 2025 at 02:55:30PM -0400, Tamir Duberstein wrote: > On Thu, Jul 3, 2025 at 11:08 AM Benno Lossin <lossin@kernel.org> wrote: > > > > 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@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@google.com> > > >> > Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467 > > >> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > >> > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > >> > Signed-off-by: Tamir Duberstein <tamird@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. > > Ah, I see what you mean. Yeah, the requirement to RB the entire patch > does mean there's a benefit to smaller patches. I often tell kernel newbies: Lots of small patches which are obviously correct. A small patch tends to be more obviously correct than a big patch. The commit message is more focused and helpful because it refers to a small chunk of code. Because the commit message is more focused, it can answer questions reviewers might ask, before they ask them. If i can spend 60 seconds looking at a patch and decide it looks correct, i'm more likely to actually look at it and give a reviewed by. If i need to find 10 minutes, it is going to get put off for a later time. Many reviewers just have a few minutes here, a few there, slotted into time between other tasks, while drinking coffee, etc. Andrew
On Thu, Jul 3, 2025 at 11:28 PM Andrew Lunn <andrew@lunn.ch> wrote: > > A small patch tends to be more obviously correct than a big patch. The > commit message is more focused and helpful because it refers to a > small chunk of code. Because the commit message is more focused, it > can answer questions reviewers might ask, before they ask them. If i Yeah, also better for smaller reverts, as well as typically easier to backport if needed, etc. Cheers, Miguel
On Thu, Jul 3, 2025 at 5:38 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Thu, Jul 3, 2025 at 11:28 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > A small patch tends to be more obviously correct than a big patch. The > > commit message is more focused and helpful because it refers to a > > small chunk of code. Because the commit message is more focused, it > > can answer questions reviewers might ask, before they ask them. If i > > Yeah, also better for smaller reverts, as well as typically easier to > backport if needed, etc. I appreciate that this advice is well-intentioned, thank you. I agree that all things being equal, small changes are better. In this particular case, there are specific downsides to splitting for its own sake which I tried to explain in previous replies: splitting the proc macro from the rest of the machinery risks forcing a reviewer to assess a chunk of code without seeing how it is used; in my experience this limits the scope of the review. Splitting by subsystem has other downsides, which I attempted to enumerate in my reply to Benno in the other fork of this discussion (let's discuss those there, please). There's also a tactical question about splitting by subsystem: are there any tools that would assist in doing this, or is it a matter of manually consulting MAINTAINERS to figure out file groupings?
On Fri, Jul 4, 2025 at 12:46 AM Tamir Duberstein <tamird@gmail.com> wrote:
>
> There's also a tactical question about splitting by subsystem: are
> there any tools that would assist in doing this, or is it a matter of
> manually consulting MAINTAINERS to figure out file groupings?
As Andrew mentioned, you can use that script, though I recommend not
fully/blindly trusting it.
Sometimes you will want to adjust things, e.g. sometimes things may be
related even if in a couple different `MAINTAINERS` entries, or you
may want to adjust the flags the script provides to filter, or you may
want to check `git log --no-merges` to see who is recently applying
patches related to that area, etc.
It is essentially the same process as when you send patches.
For instance, taking the diffstat above, and ignoring contents (i.e.
assuming all lines could just be freely split and without considering
other splits discussed to make the patches smaller first and reducing
the flag day changes), I could have done something like this:
drivers/block/rnull.rs | 2 +-
rust/kernel/block/mq.rs | 2 +-
drivers/gpu/nova-core/gpu.rs | 4 +-
rust/kernel/device.rs | 2 +-
rust/kernel/kunit.rs | 6 +--
rust/kernel/seq_file.rs | 2 +-
rust/kernel/fmt.rs | 89 +++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 1 +
rust/kernel/prelude.rs | 3 +-
rust/kernel/print.rs | 4 +-
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 +-
And then those long lines may hint that it may make sense to split the
smaller tweaks in the last group into their own patch, so that it
mirrors what is done for the other smaller groups. Thus possibly
leaving the feature being added into its own patch, which would be the
biggest and the one that would take some discussion. And the others
would be the small ones that are easy to Acked-by or Reviewed-by or
simply take (if independently possible) by other maintainers.
And so on -- again, this is speaking generally, and it is just a dummy
example, not intended to say anything about the current patch. And
sometimes things may not make sense to split too far, and it can be
more annoying than it is worth for everyone involved, e.g. when we are
talking about trivial conversions that could take 50+ patches that
could be automated instead and then applied by a single maintainer.
So it is a balance.
Cheers,
Miguel
> There's also a tactical question about splitting by subsystem: are > there any tools that would assist in doing this, or is it a matter of > manually consulting MAINTAINERS to figure out file groupings? You can run ./scripts/get_maintainer -f path/to/file.c and it will give you the Maintainers for that file. From that you can imply the subsystem. It might be possible to do one tree wide patchset, since Rust is still small at the moment. But you will need to get Reviewed-by: or Acked-by: from each subsystem Maintainer for the patches. That is not always easy, since some subsystems have CI systems, and will want the patch to pass their CI tests before giving an Reviewed-by. Andrew
On Thu Jul 3, 2025 at 8:55 PM CEST, Tamir Duberstein wrote:
> On Thu, Jul 3, 2025 at 11:08 AM Benno Lossin <lossin@kernel.org> wrote:
>> 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@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@google.com>
>> >> > Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467
>> >> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> >> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> >> > Signed-off-by: Tamir Duberstein <tamird@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.
>
> Ah, I see what you mean. Yeah, the requirement to RB the entire patch
> does mean there's a benefit to smaller patches.
>
>> 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.
>
> After implementing your suggestion of re-exporting things from
> `kernel::fmt` the diffstat is
>
> 26 files changed, 253 insertions(+), 51 deletions(-)
>
> so I guess I could do all the additions in one patch, but then
> *everything* else has to go in a single patch together because the
> formatting macros either want core::fmt::Display or
> kernel::fmt::Display; they can't work in a halfway state.
I don't understand, can't you just do:
* add `rust/kernel/fmt.rs`,
* add `rust/macros/fmt.rs`,
* change all occurrences of `core::fmt` to `kernel::fmt` and
`format_args!` to `fmt!`.
The last one could be split by subsystem, no? Some subsystems might
interact and thus need simultaneous splitting, but there should be some
independent ones.
>> > 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.
>
> Sure, I guess I will split them. But as noted above: changing the
> formatting macros and all the types' trait implementations has to be a
> "flag day" change.
See above.
>> >> > +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?
>
> In general the benefit is that the method does not ignore some portion
> of `Self`. A method that uses `self.0` would not provoke a compiler
> error in case another field is added, while this form would.
Yeah, but why would that change happen here? And even if it got another
field, why would that invalidate the impl of `fn fmt`?
---
Cheers,
Benno
On Thu, Jul 3, 2025 at 4:36 PM Benno Lossin <lossin@kernel.org> wrote:
>
> On Thu Jul 3, 2025 at 8:55 PM CEST, Tamir Duberstein wrote:
> > On Thu, Jul 3, 2025 at 11:08 AM Benno Lossin <lossin@kernel.org> wrote:
> >> 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@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@google.com>
> >> >> > Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467
> >> >> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> >> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> >> >> > Signed-off-by: Tamir Duberstein <tamird@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.
> >
> > Ah, I see what you mean. Yeah, the requirement to RB the entire patch
> > does mean there's a benefit to smaller patches.
> >
> >> 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.
> >
> > After implementing your suggestion of re-exporting things from
> > `kernel::fmt` the diffstat is
> >
> > 26 files changed, 253 insertions(+), 51 deletions(-)
> >
> > so I guess I could do all the additions in one patch, but then
> > *everything* else has to go in a single patch together because the
> > formatting macros either want core::fmt::Display or
> > kernel::fmt::Display; they can't work in a halfway state.
>
> I don't understand, can't you just do:
>
> * add `rust/kernel/fmt.rs`,
> * add `rust/macros/fmt.rs`,
> * change all occurrences of `core::fmt` to `kernel::fmt` and
> `format_args!` to `fmt!`.
Yes, such a split could be done - I will do so in the next spin
> The last one could be split by subsystem, no? Some subsystems might
> interact and thus need simultaneous splitting, but there should be some
> independent ones.
Yes, it probably can. As you say, some subsystems might interact - the
claimed benefit of doing this subsystem-by-subsystem split is that it
avoids conflicts with ongoing work that will conflict with a large
patch, but this is also the downside; if ongoing work changes the set
of interactions between subsystems then a maintainer may find
themselves unable to emit the log message they want (because one
subsystem is using kernel::fmt while another is still on core::fmt).
>
> >> > 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.
> >
> > Sure, I guess I will split them. But as noted above: changing the
> > formatting macros and all the types' trait implementations has to be a
> > "flag day" change.
>
> See above.
>
> >> >> > +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?
> >
> > In general the benefit is that the method does not ignore some portion
> > of `Self`. A method that uses `self.0` would not provoke a compiler
> > error in case another field is added, while this form would.
>
> Yeah, but why would that change happen here? And even if it got another
> field, why would that invalidate the impl of `fn fmt`?
I don't know, but I would rather force a person to make that decision
when they add another field rather than assume that such an addition
wouldn't require changes here.
On Fri Jul 4, 2025 at 12:41 AM CEST, Tamir Duberstein wrote:
> On Thu, Jul 3, 2025 at 4:36 PM Benno Lossin <lossin@kernel.org> wrote:
>> On Thu Jul 3, 2025 at 8:55 PM CEST, Tamir Duberstein wrote:
>> > On Thu, Jul 3, 2025 at 11:08 AM Benno Lossin <lossin@kernel.org> wrote:
>> >> 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@kernel.org> wrote:
>> >> >> On Tue Jul 1, 2025 at 6:49 PM CEST, Tamir Duberstein wrote:
>> >> >> > +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?
>> >
>> > In general the benefit is that the method does not ignore some portion
>> > of `Self`. A method that uses `self.0` would not provoke a compiler
>> > error in case another field is added, while this form would.
>>
>> Yeah, but why would that change happen here? And even if it got another
>> field, why would that invalidate the impl of `fn fmt`?
>
> I don't know, but I would rather force a person to make that decision
> when they add another field rather than assume that such an addition
> wouldn't require changes here.
I don't think so. If this were in another file, then destructuring
might make sense if the struct could conceivably get more fields in the
future **and** it if the other file relied on there only being one
field (or if it *had* to be changed when there was a field added). This
isn't the case here so it's just unnecessary noise.
---
Cheers,
Benno
> Yes, it probably can. As you say, some subsystems might interact - the > claimed benefit of doing this subsystem-by-subsystem split is that it > avoids conflicts with ongoing work that will conflict with a large > patch, but this is also the downside; if ongoing work changes the set > of interactions between subsystems then a maintainer may find > themselves unable to emit the log message they want (because one > subsystem is using kernel::fmt while another is still on core::fmt). This sounds like an abstraction problem. As a developer, i just want an API to print stuff. I don't care about what happens underneath. Could you add an implementation of the API which uses core:fmt underneath. Get that merged. You can then convert each subsystem one by one to use the new API. Since all you are changing is the API, not the implementation, there is no compatibility issues. Then, once all users are converted to the API, you can have one patch which flips the implementation from core:fmt to kernel:fmt. It might take you three kernel cycles to get this done, but that is relatively fast for a tree wide change, which sometimes takes years. Andrew
On Thu, Jul 3, 2025 at 6:41 PM Tamir Duberstein <tamird@gmail.com> wrote: > > On Thu, Jul 3, 2025 at 4:36 PM Benno Lossin <lossin@kernel.org> wrote: > > > > On Thu Jul 3, 2025 at 8:55 PM CEST, Tamir Duberstein wrote: > > > On Thu, Jul 3, 2025 at 11:08 AM Benno Lossin <lossin@kernel.org> wrote: > > >> 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@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@google.com> > > >> >> > Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467 > > >> >> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > >> >> > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > >> >> > Signed-off-by: Tamir Duberstein <tamird@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. > > > > > > Ah, I see what you mean. Yeah, the requirement to RB the entire patch > > > does mean there's a benefit to smaller patches. > > > > > >> 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. > > > > > > After implementing your suggestion of re-exporting things from > > > `kernel::fmt` the diffstat is > > > > > > 26 files changed, 253 insertions(+), 51 deletions(-) > > > > > > so I guess I could do all the additions in one patch, but then > > > *everything* else has to go in a single patch together because the > > > formatting macros either want core::fmt::Display or > > > kernel::fmt::Display; they can't work in a halfway state. > > > > I don't understand, can't you just do: > > > > * add `rust/kernel/fmt.rs`, > > * add `rust/macros/fmt.rs`, > > * change all occurrences of `core::fmt` to `kernel::fmt` and > > `format_args!` to `fmt!`. > > Yes, such a split could be done - I will do so in the next spin > > > > The last one could be split by subsystem, no? Some subsystems might > > interact and thus need simultaneous splitting, but there should be some > > independent ones. > > Yes, it probably can. As you say, some subsystems might interact - the > claimed benefit of doing this subsystem-by-subsystem split is that it > avoids conflicts with ongoing work that will conflict with a large > patch, but this is also the downside; if ongoing work changes the set > of interactions between subsystems then a maintainer may find > themselves unable to emit the log message they want (because one > subsystem is using kernel::fmt while another is still on core::fmt). I gave this a try. I ran into the problem that `format_args!` (and, after this patch, `fmt!`) is at the center of `print_macro!`, which itself underpins various other formatting macros. This means we'd have to bifurcate the formatting infrastructure to support an incremental migration. That's quite a bit of code, and likely quite a mess in the resulting git history -- and that's setting aside the toil required to figure out the correct combinations of subsystems that must migrate together.
On Fri Jul 4, 2025 at 1:23 AM CEST, Tamir Duberstein wrote: > On Thu, Jul 3, 2025 at 6:41 PM Tamir Duberstein <tamird@gmail.com> wrote: >> On Thu, Jul 3, 2025 at 4:36 PM Benno Lossin <lossin@kernel.org> wrote: >> > >> > I don't understand, can't you just do: >> > >> > * add `rust/kernel/fmt.rs`, >> > * add `rust/macros/fmt.rs`, >> > * change all occurrences of `core::fmt` to `kernel::fmt` and >> > `format_args!` to `fmt!`. >> >> Yes, such a split could be done - I will do so in the next spin >> >> >> > The last one could be split by subsystem, no? Some subsystems might >> > interact and thus need simultaneous splitting, but there should be some >> > independent ones. >> >> Yes, it probably can. As you say, some subsystems might interact - the >> claimed benefit of doing this subsystem-by-subsystem split is that it >> avoids conflicts with ongoing work that will conflict with a large >> patch, but this is also the downside; if ongoing work changes the set >> of interactions between subsystems then a maintainer may find >> themselves unable to emit the log message they want (because one >> subsystem is using kernel::fmt while another is still on core::fmt). > > I gave this a try. I ran into the problem that `format_args!` (and, > after this patch, `fmt!`) is at the center of `print_macro!`, which > itself underpins various other formatting macros. This means we'd have > to bifurcate the formatting infrastructure to support an incremental > migration. That's quite a bit of code, and likely quite a mess in the > resulting git history -- and that's setting aside the toil required to > figure out the correct combinations of subsystems that must migrate > together. So here is what we can do without duplicating the logic, though it requires multiple cycles: 1. We merge the two `fmt.rs` files & each subsystem merges an implementation of `kernel::fmt::Display` for their types, but keeps the `core::fmt::Display` impl around. 2. After all subsystems have merged the previous step, we change the implementations of `print_macro!` to use `fmt!` instead of `format_args!`. 3. We remove all occurrences of `core::fmt` (& replace them with `kernel::fmt`), removing the `core::fmt::Display` impls. --- Cheers, Benno
On Fri, Jul 4, 2025 at 6:09 AM Benno Lossin <lossin@kernel.org> wrote: > > On Fri Jul 4, 2025 at 1:23 AM CEST, Tamir Duberstein wrote: > > On Thu, Jul 3, 2025 at 6:41 PM Tamir Duberstein <tamird@gmail.com> wrote: > >> On Thu, Jul 3, 2025 at 4:36 PM Benno Lossin <lossin@kernel.org> wrote: > >> > > >> > I don't understand, can't you just do: > >> > > >> > * add `rust/kernel/fmt.rs`, > >> > * add `rust/macros/fmt.rs`, > >> > * change all occurrences of `core::fmt` to `kernel::fmt` and > >> > `format_args!` to `fmt!`. > >> > >> Yes, such a split could be done - I will do so in the next spin > >> > >> > >> > The last one could be split by subsystem, no? Some subsystems might > >> > interact and thus need simultaneous splitting, but there should be some > >> > independent ones. > >> > >> Yes, it probably can. As you say, some subsystems might interact - the > >> claimed benefit of doing this subsystem-by-subsystem split is that it > >> avoids conflicts with ongoing work that will conflict with a large > >> patch, but this is also the downside; if ongoing work changes the set > >> of interactions between subsystems then a maintainer may find > >> themselves unable to emit the log message they want (because one > >> subsystem is using kernel::fmt while another is still on core::fmt). > > > > I gave this a try. I ran into the problem that `format_args!` (and, > > after this patch, `fmt!`) is at the center of `print_macro!`, which > > itself underpins various other formatting macros. This means we'd have > > to bifurcate the formatting infrastructure to support an incremental > > migration. That's quite a bit of code, and likely quite a mess in the > > resulting git history -- and that's setting aside the toil required to > > figure out the correct combinations of subsystems that must migrate > > together. > > So here is what we can do without duplicating the logic, though it > requires multiple cycles: > > 1. We merge the two `fmt.rs` files & each subsystem merges an > implementation of `kernel::fmt::Display` for their types, but keeps > the `core::fmt::Display` impl around. > 2. After all subsystems have merged the previous step, we change the > implementations of `print_macro!` to use `fmt!` instead of > `format_args!`. > 3. We remove all occurrences of `core::fmt` (& replace them with > `kernel::fmt`), removing the `core::fmt::Display` impls. That would probably work. We will probably see regressions because we can't just replace `core::fmt` imports with `kernel::fmt`, so new code may appear that uses the former. I think this discussion would be productive on the next spin. The changes in other subsystems are now almost entirely changing of import paths -- perhaps that would be sufficiently uncontroversial for folks to give their Acked-bys.
On Fri, Jul 4, 2025 at 1:59 PM Tamir Duberstein <tamird@gmail.com> wrote: > > That would probably work. We will probably see regressions because we > can't just replace `core::fmt` imports with `kernel::fmt`, so new code > may appear that uses the former. That is fine -- it happens all the time with this sort of approach. Cheers, Miguel
On Fri, Jul 4, 2025 at 8:15 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Fri, Jul 4, 2025 at 1:59 PM Tamir Duberstein <tamird@gmail.com> wrote: > > > > That would probably work. We will probably see regressions because we > > can't just replace `core::fmt` imports with `kernel::fmt`, so new code > > may appear that uses the former. > > That is fine -- it happens all the time with this sort of approach. OK, with all the splitting requested, this comes out to ~54 patches. I'll send the first bit (which can go in cycle 0) as v14.
> OK, with all the splitting requested, this comes out to ~54 patches. > I'll send the first bit (which can go in cycle 0) as v14. FYI: Some subsystems have limits as to how many patches can be posted at once. e.g. for netdev, don't send more than 15. This is another rule about making it easier for reviewers. If i see 20 patches, i will defer reviewing them until later, when i have more time. For a handful of patches, especially small obvious patches, i'm more likely to look at them straight away. Andrew
On Thu, Jul 3, 2025 at 2:55 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Thu, Jul 3, 2025 at 11:08 AM Benno Lossin <lossin@kernel.org> wrote:
> >
> > 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@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@google.com>
> > >> > Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Custom.20formatting/with/516476467
> > >> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > >> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > >> > Signed-off-by: Tamir Duberstein <tamird@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.
>
> Ah, I see what you mean. Yeah, the requirement to RB the entire patch
> does mean there's a benefit to smaller patches.
>
> > 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.
>
> After implementing your suggestion of re-exporting things from
> `kernel::fmt` the diffstat is
>
> 26 files changed, 253 insertions(+), 51 deletions(-)
>
> so I guess I could do all the additions in one patch, but then
> *everything* else has to go in a single patch together because the
> formatting macros either want core::fmt::Display or
> kernel::fmt::Display; they can't work in a halfway state.
>
> >
> > > 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.
>
> Sure, I guess I will split them. But as noted above: changing the
> formatting macros and all the types' trait implementations has to be a
> "flag day" change.
>
> >
> > >> > 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?
>
> I didn't find anything suitable. Do you have one in mind?
I think we want https://github.com/rust-lang/rust-clippy/issues/14807.
© 2016 - 2025 Red Hat, Inc.