Andrew Lunn
2025-Jul-03 21:28 UTC
[PATCH v13 2/5] rust: support formatting of foreign types
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 at 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 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. > > 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
Miguel Ojeda
2025-Jul-03 21:38 UTC
[PATCH v13 2/5] rust: support formatting of foreign types
On Thu, Jul 3, 2025 at 11:28?PM Andrew Lunn <andrew at 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 iYeah, also better for smaller reverts, as well as typically easier to backport if needed, etc. Cheers, Miguel
Tamir Duberstein
2025-Jul-03 22:46 UTC
[PATCH v13 2/5] rust: support formatting of foreign types
On Thu, Jul 3, 2025 at 5:38?PM Miguel Ojeda <miguel.ojeda.sandonis at gmail.com> wrote:> > On Thu, Jul 3, 2025 at 11:28?PM Andrew Lunn <andrew at 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?