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 <[email protected]>
---
Changes in V4:
- move to_ascii_{lower,upper}case() to CStr
- add a few comments suggested by Alice
Changes in V3:
- add an `impl DerefMut for CString`, such that these functions can be defined
for `CStr` as `&mut self` and still be called on a `CString`
Changes in V2:
- expand commit message mentioning the use case
- expand function doc comments to match the ones from Rust's stdlib
- rename to_* to make_* and add the actual to_* implementations
---
rust/kernel/str.rs | 87 +++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 86 insertions(+), 1 deletion(-)
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index 7d848b83add4..4dec89455e2d 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -5,7 +5,7 @@
use alloc::alloc::AllocError;
use alloc::vec::Vec;
use core::fmt::{self, Write};
-use core::ops::{self, Deref, Index};
+use core::ops::{self, Deref, DerefMut, Index};
use crate::{
bindings,
@@ -143,6 +143,19 @@ pub const fn from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, CStrConvertError
unsafe { core::mem::transmute(bytes) }
}
+ /// Creates a mutable [`CStr`] from a `[u8]` without performing any
+ /// additional checks.
+ ///
+ /// # Safety
+ ///
+ /// `bytes` *must* end with a `NUL` byte, and should only have a single
+ /// `NUL` byte (or the string will be truncated).
+ #[inline]
+ pub unsafe fn from_bytes_with_nul_unchecked_mut(bytes: &mut [u8]) -> &mut CStr {
+ // SAFETY: Properties of `bytes` guaranteed by the safety precondition.
+ unsafe { &mut *(bytes as *mut [u8] as *mut CStr) }
+ }
+
/// Returns a C pointer to the string.
#[inline]
pub const fn as_char_ptr(&self) -> *const core::ffi::c_char {
@@ -206,6 +219,70 @@ pub unsafe fn as_str_unchecked(&self) -> &str {
pub fn to_cstring(&self) -> Result<CString, AllocError> {
CString::try_from(self)
}
+
+ /// Converts this [`CStr`] to its ASCII lower case equivalent in-place.
+ ///
+ /// ASCII letters 'A' to 'Z' are mapped to 'a' to 'z',
+ /// but non-ASCII letters are unchanged.
+ ///
+ /// To return a new lowercased value without modifying the existing one, use
+ /// [`to_ascii_lowercase()`].
+ ///
+ /// [`to_ascii_lowercase()`]: #method.to_ascii_lowercase
+ pub fn make_ascii_lowercase(&mut self) {
+ // INVARIANT: This doesn't introduce or remove NUL bytes in the C
+ // string.
+ self.0.make_ascii_lowercase();
+ }
+
+ /// Converts this [`CStr`] to its ASCII upper case equivalent in-place.
+ ///
+ /// ASCII letters 'a' to 'z' are mapped to 'A' to 'Z',
+ /// but non-ASCII letters are unchanged.
+ ///
+ /// To return a new uppercased value without modifying the existing one, use
+ /// [`to_ascii_uppercase()`].
+ ///
+ /// [`to_ascii_uppercase()`]: #method.to_ascii_uppercase
+ pub fn make_ascii_uppercase(&mut self) {
+ // INVARIANT: This doesn't introduce or remove NUL bytes in the C
+ // string.
+ self.0.make_ascii_uppercase();
+ }
+
+ /// Returns a copy of this [`CString`] where each character is mapped to its
+ /// ASCII lower case equivalent.
+ ///
+ /// ASCII letters 'A' to 'Z' are mapped to 'a' to 'z',
+ /// but non-ASCII letters are unchanged.
+ ///
+ /// To lowercase the value in-place, use [`make_ascii_lowercase`].
+ ///
+ /// [`make_ascii_lowercase`]: str::make_ascii_lowercase
+ pub fn to_ascii_lowercase(&self) -> Result<CString, AllocError> {
+ let mut s = self.to_cstring()?;
+
+ s.make_ascii_lowercase();
+
+ return Ok(s);
+ }
+
+ /// Returns a copy of this [`CString`] where each character is mapped to its
+ /// ASCII upper case equivalent.
+ ///
+ /// ASCII letters 'a' to 'z' are mapped to 'A' to 'Z',
+ /// but non-ASCII letters are unchanged.
+ ///
+ /// To uppercase the value in-place, use [`make_ascii_uppercase`].
+ ///
+ /// [`make_ascii_uppercase`]: str::make_ascii_uppercase
+ pub fn to_ascii_uppercase(&self) -> Result<CString, AllocError> {
+ let mut s = self.to_cstring()?;
+
+ s.make_ascii_uppercase();
+
+ return Ok(s);
+ }
}
impl fmt::Display for CStr {
@@ -593,6 +670,14 @@ fn deref(&self) -> &Self::Target {
}
}
+impl DerefMut for CString {
+ fn deref_mut(&mut self) -> &mut Self::Target {
+ // SAFETY: A `CString` is always NUL-terminated and contains no other
+ // NUL bytes.
+ unsafe { CStr::from_bytes_with_nul_unchecked_mut(&mut *self.buf) }
+ }
+}
+
impl<'a> TryFrom<&'a CStr> for CString {
type Error = AllocError;
base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d
--
2.43.0
> 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 <[email protected]>
This looks good to me, so you may add my
Reviewed-by: Alice Ryhl <[email protected]>
However, it looks like this patch has some clippy warnings that need to
be fixed before it can be merged.
$ make LLVM=1 CLIPPY=1
error: unneeded `return` statement
--> 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
= 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
--> 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
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 <[email protected]>
>
> This looks good to me, so you may add my
>
> Reviewed-by: Alice Ryhl <[email protected]>
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
>
On Tue, Feb 20, 2024 at 1:02 PM Danilo Krummrich <[email protected]> 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 <[email protected]>
> >
> > This looks good to me, so you may add my
> >
> > Reviewed-by: Alice Ryhl <[email protected]>
>
> 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
On 2/20/24 14:17, Alice Ryhl wrote:
> On Tue, Feb 20, 2024 at 1:02 PM Danilo Krummrich <[email protected]> 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 <[email protected]>
>>>
>>> This looks good to me, so you may add my
>>>
>>> Reviewed-by: Alice Ryhl <[email protected]>
>>
>> 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
>
On Tue, Feb 20, 2024 at 1:03 PM Danilo Krummrich <[email protected]> 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
On Tue, Feb 20, 2024 at 3:53 PM Danilo Krummrich <[email protected]> wrote:
>
> Just to clarify, I did not say anything else. As mentioned, I think those
> should not even be warnings.
Well, for things like the `return` one, I find it unlikely it will change.
And for other things that there are 2 ways of doing it, what we
typically want is to have one way of doing it, rather than allowing
both ways.
> 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...
The scope is already defined -- it is global, precisely because we
want to keep all kernel code as consistent as possible.
> Is there any objective reason not to be allowed to be explicit here?
What is not objective about wanting to be consistent? How is your
argument objective if ours isn't?
> Well, I generally agree. However, I'm clearly against *blindly* following
> formatters and linters.
>
> 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.
Who is *blindly* following formatters and linters? We don't have
objective reasons?
I don't appreciate the way you are wording things here.
Does it actually lead to "less ideal code", or the opposite?
Cheers,
Miguel
On 2/20/24 16:04, Miguel Ojeda wrote:
> On Tue, Feb 20, 2024 at 1:03 PM Danilo Krummrich <[email protected]> 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
>
On Tue, Feb 20, 2024 at 4:53 PM Danilo Krummrich <[email protected]> wrote:
>
> The rational for a convention can't be that it is a convention. Instead
> it should be a convention for an objective reason.
The rationale __for the lint__ is that it is a very established
convention in Rust code.
That is what Clippy is telling you.
You may not agree with the convention (and thus the lint). That is
fine, but it is still a fact that it is the convention, and that is
why I said whoever wrote that Clippy description probably felt that
wording is good enough.
> I'm not saying that we should enforce it otherwise, I just think that we
> should have objective reasons for restrictions.
Again, you seem to be saying we do not have objective reasons.
> However, I also don't see why we should disallow it as long as there is
> no objective reason to do so.
There are objective reasons to do so and we already explained them to you above:
https://lore.kernel.org/rust-for-linux/CAH5fLggXiXGA_UWCxqqLhMpXe1kepDv2vMG+1jLGaDSzdRHvSw@mail.gmail.com/
https://lore.kernel.org/rust-for-linux/CANiq72nVkV3+1rt4Mi+Own6KGAzmvR2jf8fFsp9NBu_gy_ob5g@mail.gmail.com/
You disagree? Fine, if you can come up with an argument that convinces
us or a good majority of the kernel that uses Rust, then I will
happily apply the patch.
Personally, I don't particularly care (based on style arguments alone)
whether to write `return` or not. But I do care about not wasting time
on litigating particular choices each time somebody does not like one.
> 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. :-)
>
> The language has various characteristics, maybe that's why it allows both?
Languages may allow a lot of things, but that does not mean we write
the code like IOCCC just because we can.
Nor it does mean that a project should allow every way to write things
either, _especially_ when it is a very widespread convention.
> See above.
I don't see it, sorry. You said you want to be "explicit", but one can
very easily argue you are _not_ being explicit by omitting the
`return` in some cases.
> 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?
We already told you in this thread several times -- the `return` one
is idiomatic and so on. See the links above.
> 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.
Why? To me, we should have good examples and an objective rationale
for things where we actually want to deviate from what everybody else
is doing.
Cheers,
Miguel
On 2/20/24 16:45, Miguel Ojeda wrote:
> On Tue, Feb 20, 2024 at 3:53 PM Danilo Krummrich <[email protected]> wrote:
>>
>> Just to clarify, I did not say anything else. As mentioned, I think those
>> should not even be warnings.
>
> Well, for things like the `return` one, I find it unlikely it will change.
>
> And for other things that there are 2 ways of doing it, what we
> typically want is to have one way of doing it, rather than allowing
> both ways.
>
>> 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...
>
> The scope is already defined -- it is global, precisely because we
> want to keep all kernel code as consistent as possible.>
>> Is there any objective reason not to be allowed to be explicit here?
>
> What is not objective about wanting to be consistent? How is your
> argument objective if ours isn't?
Nothing, but I also never wrote that wanting to be consistent is not objective.
I wrote that omitting the return statement is more "rusty" isn't an objective
argument in my opinion.
Maybe the misunderstanding is that consistent doesn't necessarily mean to do what
everyone else does. Consistency has a scope. For instance, we can also be consistent
with something that everyone else does differently, like linked lists throughout the
kernel.
>
>> Well, I generally agree. However, I'm clearly against *blindly* following
>> formatters and linters.
>>
>> 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.
>
> Who is *blindly* following formatters and linters? We don't have
> objective reasons?
This doesn't seem to be a "straight up" question. Hence, for the following I will
assume that you feel like I said those things about you.
First of all, I said that *we* (as a community) should not blindly follow formatters
and linters and should have objective reasons for language restrictions in general.
I never said anyone specific is doing that. Claiming that I did so, *would* be a straw
man.
Furthermore I was questioning two of the restrictions we already have and was asking
for an objective rationale for those, since the clipply documentation does not provide
an objective rationale in my opinion.
Generally speaking, I don't think there is anything wrong in saying that I think some
argument isn't objective (even if I'd be proven wrong). This is by far not the
same as saying someone has no objective reasons (in general). This would be a straw man
as well.
>
> I don't appreciate the way you are wording things here.
I already had the feeling from your answer in the other thread. But, unfortunately,
I don't understand why. If the above did not convince you otherwise, can you please
explain?
>
> Does it actually lead to "less ideal code", or the opposite?
>
> Cheers,
> Miguel
>
On 2/20/24 17:53, Miguel Ojeda wrote:
> On Tue, Feb 20, 2024 at 4:53 PM Danilo Krummrich <[email protected]> wrote:
>>
>> The rational for a convention can't be that it is a convention. Instead
>> it should be a convention for an objective reason.
>
> The rationale __for the lint__ is that it is a very established
> convention in Rust code.
Understood. I'm basically just asking why this is the convention.
Because I assume that there must be a good reason for that. If there is none,
and it's really just because everyone is doing it, I personally think that's
not an objective rationale.
If there is no other reason it could even be just the opposite causality, as in
it became the convention because clippy enforces it. (Disclaimer: I really don't
know and so far I have no reason to assume so.)
Generally, I don't think there is anything wrong with questioning things and I
also don't think there is anything wrong in not accepting "because everyone does
so" as an objective rationale.
Otherwise I don't see any disagreement. I also understood that you want to be
consistent and comply with this convention and surely I accept that.
But again, I also think it's perfectly valid questioning things.
>
> That is what Clippy is telling you.
>
> You may not agree with the convention (and thus the lint). That is
> fine, but it is still a fact that it is the convention, and that is
> why I said whoever wrote that Clippy description probably felt that
> wording is good enough.
>
>> I'm not saying that we should enforce it otherwise, I just think that we
>> should have objective reasons for restrictions.
>
> Again, you seem to be saying we do not have objective reasons.
I'm honestly sorry about this misunderstanding and that this seems to be an
emotional discussion for you.
I never said that you don't have objective reasons (in general). I just said that
I don't consider *one specific rationale* as objective (or factual).
And I think it's perfectly valid to claim the right to do so. This isn't a personal
attack in any way.