Message-ID: <e543b270-dea7-477a-b83d-62129d4ac708@redhat.com>
Date: Tue, 20 Feb 2024 13:02:50 +0100
From: Danilo Krummrich <dakr@...hat.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: a.hindborg@...sung.com, alex.gaynor@...il.com, benno.lossin@...ton.me,
 bjorn3_gh@...tonmail.com, boqun.feng@...il.com, gary@...yguo.net,
 linux-kernel@...r.kernel.org, ojeda@...nel.org,
 rust-for-linux@...r.kernel.org, wedsonaf@...il.com
Subject: Re: [PATCH v4] rust: str: add {make,to}_{upper,lower}case() to
 CString

On 2/20/24 10:35, Alice Ryhl wrote:
>> Add functions to convert a CString to upper- / lowercase, either
>> in-place or by creating a copy of the original CString.
>>
>> Naming followes the one from the Rust stdlib, where functions starting
>> with 'to' create a copy and functions starting with 'make' perform an
>> in-place conversion.
>>
>> This is required by the Nova project (GSP only Rust successor of
>> Nouveau) to convert stringified enum values (representing different GPU
>> chipsets) to strings in order to generate the corresponding firmware
>> paths. See also [1].
>>
>> [1] https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/String.20manipulation.20in.20kernel.20Rust
>>
>> Signed-off-by: Danilo Krummrich <dakr@...hat.com>
> 
> This looks good to me, so you may add my
> 
> Reviewed-by: Alice Ryhl <aliceryhl@...gle.com>

Thanks for reviewing this patch.

> 
> However, it looks like this patch has some clippy warnings that need to
> be fixed before it can be merged.

It seems that those warnings are treated as fatal even, although, given the
rationale for these warnings, I'm not even sure those should be warnings at
all.

> 
> $ make LLVM=1 CLIPPY=1
> error: unneeded `return` statement

I fail to see why being explicit is a bad thing in this context, and even more
why this is treated as error.

>     --> rust/kernel/str.rs:267:9
>      |
> 267 |         return Ok(s);
>      |         ^^^^^^^^^^^^
>      |
>      = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return

Following this link the given rationale is:

"Removing the return and semicolon will make the code more rusty."

That's the worst rationale I could think of. Without further rationale what that
should mean and why this would be good, it's entirely meaningless.

Instead, I'd argue that keeping it gives kernel people, who necessarily need to
deal with both, Rust *and* C, more consistency in kernel code.

At least, this shouldn't be fatal IMHO.

>      = note: `-D clippy::needless-return` implied by `-D clippy::style`
>      = help: to override `-D clippy::style` add `#[allow(clippy::needless_return)]`
> help: remove `return`
>      |
> 267 -         return Ok(s);
> 267 +         Ok(s)
>      |
> 
> error: unneeded `return` statement
>     --> rust/kernel/str.rs:284:9
>      |
> 284 |         return Ok(s);
>      |         ^^^^^^^^^^^^
>      |
>      = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return
> help: remove `return`
>      |
> 284 -         return Ok(s);
> 284 +         Ok(s)
>      |
> 
> error: deref which would be done by auto-deref

Similar story here. Why is it bad, and even *fatal*, to be explicit?

The answer from the link below: "This unnecessarily complicates the code."

Again, not a great rationale, this is entirely subjective and might even depend
on the context of the project. Again, for kernel people who need to deal with Rust
*and* C continuously it might be better to be explicit.

>     --> rust/kernel/str.rs:677:58
>      |
> 677 |         unsafe { CStr::from_bytes_with_nul_unchecked_mut(&mut *self.buf) }
>      |                                                          ^^^^^^^^^^^^^^ help: try: `&mut self.buf`
>      |
>      = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#explicit_auto_deref
>      = note: `-D clippy::explicit-auto-deref` implied by `-D clippy::complexity`
>      = help: to override `-D clippy::complexity` add `#[allow(clippy::explicit_auto_deref)]`
> 
> error: aborting due to 3 previous errors
> 
> Alice
> 

Powered by blists - more mailing lists