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: <462aad75-4f03-4f8b-ad58-eef429ed2b34@redhat.com>
Date: Tue, 20 Feb 2024 16:53:16 +0100
From: Danilo Krummrich <dakr@...hat.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.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 2/20/24 16:04, Miguel Ojeda wrote:
> 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 rational for a convention can't be that it is a convention. Instead
it should be a convention for an objective reason.

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

I'm not saying that we should enforce it otherwise, I just think that we
should have objective reasons for restrictions.

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

No, I didn't say, nor did I mean, that we should align with C in general,
nor should it be enforced.

However, I also don't see why we should disallow it as long as there is
no objective reason to do so.

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

That's actually what the language did already with early-return vs return at
the end of the function.

I admit that consistent inconsistency is also kinda consistent though. :-)

>    - Rust is a more expression-oriented language than C.

The language has various characteristics, maybe that's why it allows both?

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

See above.

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

Then it shouldn't be a warning either IMHO.

> 
>> Similar story here. Why is it bad, and even *fatal*, to be explicit?
> 
> This one is more arguable, and could be discussed.

That's great, although I really don't understand why you think this one is, but
the other one isn't. What's the difference?

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

Thanks for the invitation, I'm happy to join!

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

I agree, but I also think it should be the other way around. We should have good
examples and an objective rationale for things we restrict.

> 
> Cheers,
> Miguel
>