Message-ID: <19d0a4a9-85bb-4b9e-96b0-338adebc90e5@redhat.com>
Date: Tue, 20 Feb 2024 15:53:27 +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 14:17, Alice Ryhl wrote:
> On Tue, Feb 20, 2024 at 1:02 PM Danilo Krummrich <dakr@...hat.com> wrote:
>>
>> 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.
> 
> The build currently succeeds with zero warnings. That's a very useful
> property, and I would not like to give it up.

Just to clarify, I did not say anything else. As mentioned, I think those
should not even be warnings.

> 
> You could submit a patch to silence these specific warnings. The

I'm happy to do that. We should define the scope for that though. I think this
should be set globally, or at least not per crate. However, I don't really know
what's the best way to do that. We could pass '-Aclippy::' to the compiler...

> clippy::explicit_auto_deref warning is not one I care for, but I would
> object to silencing clippy::needless_return. Using return statements
> when you are not doing an early-return really is extremely unidiomatic
> Rust.

Unfortunately, that's just a different version of:

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

Hence, I still fail to see why being explicit is a bad thing in this context.
Is there any objective reason not to be allowed to be explicit here?

> 
> Ultimately, I think there is a lot of value of just applying the code
> formatters and linters and following them to zero warnings. It ensures
> that the Rust code is relatively idiomatic, helps avoid long
> unproductive discussions, and makes it easy for me to fix formatting
> issues in my own patches (just run clippy and rustfmt on everything,
> and it lists only things that are my own fault).

Well, I generally agree. However, I'm clearly against *blindly* following
formatters and linters.

We should carefully pick the rules we follow, and we should do that for objective
reasons.

Forcing guidelines we don't have objective reasons for will otherwise just annoy
people and lead to less ideal code for the project. And I intentionally say "for
the project", since this context is important.

[1] https://rust-lang.github.io/rust-clippy/master/index.html#/needless_return
> 
> Alice
> 

Powered by blists - more mailing lists