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: <CANiq72nVkV3+1rt4Mi+Own6KGAzmvR2jf8fFsp9NBu_gy_ob5g@mail.gmail.com>
Date: Tue, 20 Feb 2024 16:04:20 +0100
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Danilo Krummrich <dakr@...hat.com>
Cc: Alice Ryhl <aliceryhl@...gle.com>, 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:03 PM Danilo Krummrich <dakr@...hat.com> wrote:
>
> 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.

Probably whoever wrote that did not feel the need to explain further
because it is the convention, but please feel free to open an issue/PR
to Clippy about improving the wording of that text.

The convention itself, however, you will find way harder to change
everywhere else.

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

That sounds to me like trying to keep consistency in style/formatting
between two languages, which is something we have discussed quite a
few times in the past.

We are keeping Rust code as idiomatic as possible, except where it may
actually make sense to diverge for kernel reasons.

But this one does not seem to be the case:

  - It is inconsistent with most Rust code out there.
  - It is inconsistent with all Rust kernel code.
  - It is inconsistent with learning material, which kernel developers use too.
  - It introduces 2 ways for writing the same trivial thing.
  - Rust is a more expression-oriented language than C.

And, by the way, your patch does use both ways. Why aren't you
explicit when it is a single expression too?

> At least, this shouldn't be fatal IMHO.

For some of the compiler-based (i.e. not Clippy) and that may make
prototyping a bad experience, I could agree (e.g. like missing
documentation is already a warning).

But please note that patches must be warning free anyway, so it is not
like this patch would have been OK.

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

This one is more arguable, and could be discussed. In fact, we planned
going through some of the lints in a meeting to see, mostly, what
extra lints could be enabled etc. You are welcome to join if that
happens (I think Trevor wanted to drive that discussion).

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

That is fine, but to decide on this like this, we need better examples
and rationale than just "it might be better" (and please note that
whatever Clippy says is not important, so complaining about their docs
being lacking is not really an argument to change kernel code).

Cheers,
Miguel