block, rust: simplify validate_block_size() function
Subsystems (2)
Hi Alexy,
Thanks for your patch. I think I understand why you would suggest the change, with you strong C background. I would prefer that we do not apply this change, see below.
Quoted 8 lines · Alexey Dobriyan
Quoted 5 lines · Benno Lossin
Quoted 1 line · Alexey Dobriyan
Using range and
contains()method is just fancy shmancy way of writingThis language doesn't fit into a commit message. Please give a technical reason to change this.
Oh come on!
Could you elaborate?
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
two comparisons. Using range doesn't prevent any bugs here because typing "=" in range can be forgotten just as easily as in "<=" operator.
I don't think that using traditional comparisons is an improvement.
They are an improvement, or rather
contains()on integers is of dubious value.
I would disagree. To me, and probably to many people who are experienced in Rust code, the range.contains() formulation is much more clear.
Quoted 2 lines
First, coding style mandates that there are no whitespace on both sides of "..". This merges all characters into one Perl-like line noise.
I don't think it looks like noise or Perl. But I am not that experienced in Perl 🤷
What code style are you referring to? We use rustfmt default settings as code style, although I am not sure if that is written down anywhere.
Quoted 2 lines
Second, when writing C I've a habit of writing comparisons like numeric line in school which goes from left to right:
But this is not C. In Rust we have other options.
Quoted 10 lines
512 ... size .. PAGE_SIZE ------> infinitySee?
Now it is easy to insert comparisons:512 <= size <= PAGE_SIZEOf course in C the middle variable must be duplicated but so what?
Quoted 7 lines
How hard is to parse this?
512 <= size && size <= PAGE_SIZEAnd thirdly, to a C/C++ dev, passing u32 by reference instead of by value to a function which obviously doesn't mutate it screams WHAT???
It might look a little funny, but in general lookups take references to the key you are searching for. It makes sense for a larger set of types. In this particular case, I don't think codegen is any different due to the reference.
Quoted 6 lines
Quoted 3 lines
When
usingcontains, both of the bounds are visible with one look.Yes, they are within 4 characters of each other 2 of which are whitespace.
I like whitespace. I think it helps make the code more readable.
Quoted 2 lines
This is what this patch is all about:
contains()for integers? I can understandcontains()instead ofstrstr()but for integers?
To me it makes sense to check if a number is in a range with contains. I appreciate that it might not make sense to you, since it is not an option in C.
Quoted 6 lines
Quoted 4 lines
When
using two comparisons, you have to first parse that they compare the same variable and then look at the bounds.Yes but now you have to parse () and .. and &.
Reading Rust takes a bit of practice. Just like reading C takes some practice to people who have not done it before.
Quoted 9 lines
Quoted 5 lines
Quoted 2 lines
Also delete few comments of "increment i by 1" variety.
As Miguel already said, these are part of the documentation. Do not remove them.
Kernel has its fair share of 1:1 kernel-doc comments which contain exactly zero useful information because everything is in function signature already.
The comment is useful to a person browsing the documentation in the HTML format. It is available here [1] if you want to have a look.
Best regards,
Andreas
Hi Andreas,
Quoted 24 lines · Andreas Hindborg
Hi Alexy,
Thanks for your patch. I think I understand why you would suggest the change, with you strong C background. I would prefer that we do not apply this change, see below.
Quoted 8 lines · Alexey Dobriyan
Quoted 5 lines · Benno Lossin
Quoted 1 line · Alexey Dobriyan
Using range and
contains()method is just fancy shmancy way of writingThis language doesn't fit into a commit message. Please give a technical reason to change this.
Oh come on!
Could you elaborate?
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
two comparisons. Using range doesn't prevent any bugs here because typing "=" in range can be forgotten just as easily as in "<=" operator.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
I don't think that using traditional comparisons is an improvement.
They are an improvement, or rather
contains()on integers is of dubious value.I would disagree. To me, and probably to many people who are experienced in Rust code, the
range.contains()formulation is much more clear.
If you want to keep dividing into Rust-land and C-land I'm afraid you will have 2 islands that do not talk to each other. I really want to be able to parse the things quickly and not constantly think if my Rust is idiomatic enough or I could write the code in a more idiomatic way with something brand new that just got off the nightly list and moved into stable.
Quoted 48 lines
Quoted 3 lines
First, coding style mandates that there are no whitespace on both sides of "..". This merges all characters into one Perl-like line noise.
I don't think it looks like noise or Perl. But I am not that experienced in Perl 🤷
What code style are you referring to? We use
rustfmtdefault settings as code style, although I am not sure if that is written down anywhere.Quoted 2 lines
Second, when writing C I've a habit of writing comparisons like numeric line in school which goes from left to right:
But this is not C. In Rust we have other options.
Quoted 10 lines
512 ... size .. PAGE_SIZE ------> infinitySee?
Now it is easy to insert comparisons:512 <= size <= PAGE_SIZEOf course in C the middle variable must be duplicated but so what?
Quoted 7 lines
How hard is to parse this?
512 <= size && size <= PAGE_SIZEAnd thirdly, to a C/C++ dev, passing u32 by reference instead of by value to a function which obviously doesn't mutate it screams WHAT???
It might look a little funny, but in general lookups take references to the key you are searching for. It makes sense for a larger set of types. In this particular case, I don't think codegen is any different due to the reference.
Quoted 6 lines
Quoted 3 lines
When
usingcontains, both of the bounds are visible with one look.Yes, they are within 4 characters of each other 2 of which are whitespace.
I like whitespace. I think it helps make the code more readable.
Quoted 6 lines
Quoted 2 lines
This is what this patch is all about:
contains()for integers? I can understandcontains()instead ofstrstr()but for integers?To me it makes sense to check if a number is in a range with
contains. I appreciate that it might not make sense to you, since it is not an option in C.
I am sure we could come up with something like:
if (!contains(MAKE_RANGE_INCL(512, 4096), size)) ...
but even if something possible it does not mean it needs to be done.
Quoted 17 lines
Quoted 7 lines
Quoted 5 lines
When
using two comparisons, you have to first parse that they compare the same variable and then look at the bounds.Yes but now you have to parse () and .. and &.
Reading Rust takes a bit of practice. Just like reading C takes some practice to people who have not done it before.
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
Also delete few comments of "increment i by 1" variety.
As Miguel already said, these are part of the documentation. Do not remove them.
Quoted 6 lines
Quoted 3 lines
Kernel has its fair share of 1:1 kernel-doc comments which contain exactly zero useful information because everything is in function signature already.
The comment is useful to a person browsing the documentation in the HTML format. It is available here [1] if you want to have a look.
This is a private function and an implementation detail. Why does it need to be exposed in documentation at all?
Thanks.
Signature · 1 line
Dmitry
Quoted 3 lines · Dmitry Torokhov
If you want to keep dividing into Rust-land and C-land I'm afraid you will have 2 islands that do not talk to each other. I really want to be
We are not trying to divide the Rust and C side, quite the opposite. That should be obvious since dividing both sides only hurts the project to begin with.
Quoted 4 lines
able to parse the things quickly and not constantly think if my Rust is idiomatic enough or I could write the code in a more idiomatic way with something brand new that just got off the nightly list and moved into stable.
If a feature is in the minimum support version we have for Rust in the kernel, and it improves the way we write code, then we should consider taking advantage of it.
Now, that particular function call would have compiled since Rust 1.35 and ranges were already a concept back in Rust 1.0. So I am not sure why you mention recently stabilized features here.
For this particular case, I don't think it matters too much, and I can see arguments both ways (and we could introduce other ways to avoid the reference or swap the order, e.g. n.within(a..b)).
Quoted 2 lines
This is a private function and an implementation detail. Why does it need to be exposed in documentation at all?
That is a different question -- but even if it should be a private function, it does not mean documentation should be removed (even if currently we do not require documentation for private items).
Cheers,
Miguel
Quoted 15 lines · Miguel Ojeda
Quoted 3 lines · Dmitry Torokhov
If you want to keep dividing into Rust-land and C-land I'm afraid you will have 2 islands that do not talk to each other. I really want to be
We are not trying to divide the Rust and C side, quite the opposite. That should be obvious since dividing both sides only hurts the project to begin with.
Quoted 4 lines
able to parse the things quickly and not constantly think if my Rust is idiomatic enough or I could write the code in a more idiomatic way with something brand new that just got off the nightly list and moved into stable.
Quoted 7 lines
If a feature is in the minimum support version we have for Rust in the kernel, and it improves the way we write code, then we should consider taking advantage of it.
Now, that particular function call would have compiled since Rust 1.35 and ranges were already a concept back in Rust 1.0. So I am not sure why you mention recently stabilized features here.
I was talking in general, not about this exact case, sorry if I was unclear. But in general I personally have this issue with Rust and to extent also with C++ where I constantly wonder if my code is "idiomatic enough" or if it looks obsolete because it is "only" C++14 and not 17 or 20.
With C usually have no such concerns which allows me to concentrate on different things.
Quoted 11 lines
For this particular case, I don't think it matters too much, and I can see arguments both ways (and we could introduce other ways to avoid the reference or swap the order, e.g.
n.within(a..b)).Quoted 2 lines
This is a private function and an implementation detail. Why does it need to be exposed in documentation at all?
That is a different question -- but even if it should be a private function, it does not mean documentation should be removed (even if currently we do not require documentation for private items).
I think exposing documentation for private function that can change at any time and is not callable from outside has little value. That does not mean that comments annotating such function have no value. But they need to be taken together with the function code, and in this case Alexey's concern about comments like "this increments that by 1" becomes quite valid.
Thanks.
Signature · 1 line
Dmitry
Quoted 16 lines · Dmitry Torokhov
Quoted 7 lines · Miguel Ojeda
Quoted 4 lines · Dmitry Torokhov
able to parse the things quickly and not constantly think if my Rust is idiomatic enough or I could write the code in a more idiomatic way with something brand new that just got off the nightly list and moved into stable.
Quoted 7 lines
If a feature is in the minimum support version we have for Rust in the kernel, and it improves the way we write code, then we should consider taking advantage of it.
Now, that particular function call would have compiled since Rust 1.35 and ranges were already a concept back in Rust 1.0. So I am not sure why you mention recently stabilized features here.
Quoted 8 lines
I was talking in general, not about this exact case, sorry if I was unclear. But in general I personally have this issue with Rust and to extent also with C++ where I constantly wonder if my code is "idiomatic enough" or if it looks obsolete because it is "only" C++14 and not 17 or 20.
With C usually have no such concerns which allows me to concentrate on different things.
I personally don't worry about whether my code is idiomatic. I just code and when others give suggestions that sound good, I apply them. There is nobody requiring all Rust code to be as idiomatic as possible. In the review process you might be nudged to write in a more idiomatic style, but that should also be true for C. The kernel is constantly adding new ways of doing things and while the language itself doesn't evolve, the codebase surely does.
Quoted 11 lines
Quoted 10 lines
For this particular case, I don't think it matters too much, and I can see arguments both ways (and we could introduce other ways to avoid the reference or swap the order, e.g.
n.within(a..b)).Quoted 2 lines
This is a private function and an implementation detail. Why does it need to be exposed in documentation at all?
That is a different question -- but even if it should be a private function, it does not mean documentation should be removed (even if currently we do not require documentation for private items).
Quoted 6 lines
I think exposing documentation for private function that can change at any time and is not callable from outside has little value. That does not mean that comments annotating such function have no value. But they need to be taken together with the function code, and in this case Alexey's concern about comments like "this increments that by 1" becomes quite valid.
For the private validate_block_size function, I can see your argument. However, I don't think that the function will change any time soon, so I don't think it's a huge issue.
---
Cheers,
Benno
Quoted 9 lines · Dmitry Torokhov
I was talking in general, not about this exact case, sorry if I was unclear. But in general I personally have this issue with Rust and to extent also with C++ where I constantly wonder if my code is "idiomatic enough" or if it looks obsolete because it is "only" C++14 and not 17 or 20.
With C usually have no such concerns which allows me to concentrate on different things.
Yeah, I see the concern, and I agree that C++ later standards can be quite daunting to keep up with (and especially to learn new pitfalls around UB in new features, e.g. C++ ranges).
With Rust, I think it is not that bad, at least so far and especially for minor features -- generally they are well thought-out and fairly regular, and at least for safe code the UB worry is not there, so it is easier to feel confident in using them.
There are some major features, like async, that we may need to carefully consider indeed though.
Quoted 6 lines
I think exposing documentation for private function that can change at any time and is not callable from outside has little value. That does not mean that comments annotating such function have no value. But they need to be taken together with the function code, and in this case Alexey's concern about comments like "this increments that by 1" becomes quite valid.
To be clear, we are not exposing the documentation in the rendered form for private items. It is possible to do so though, but we don't enable that at the time being. Although I think it would be valuable to have a "toggle" to show it.
I mean, sure, for private trivial functions, it may make not much sense to add due to the burden of writing and maintaining it. That is why we don't require documentation on private items so far, but we may want to nevertheless in the future for particular core APIs (so we could enable it in certain crates, for instance).
Thanks for taking a look, by the way.
Cheers,
Miguel