linux-kernel - Re: [PATCH v4] rust: str: add {make,to}

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAH5fLggXiXGA_UWCxqqLhMpXe1kepDv2vMG+1jLGaDSzdRHvSw@mail.gmail.com>
Date: Tue, 20 Feb 2024 14:17:22 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Danilo Krummrich <dakr@...hat.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 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.

You could submit a patch to silence these specific warnings. The
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.

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).

Alice