Re: [PATCH RESEND] block, rust: simplify validate_block_size() function

[Andreas Hindborg <nmi@metaspace.dk>]

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.

Alexey Dobriyan <adobriyan@gmail.com> writes:

> On Sat, Aug 31, 2024 at 08:39:45PM +0000, Benno Lossin wrote:
>> On 31.08.24 22:15, Alexey Dobriyan wrote:
>> > Using range and contains() method is just fancy shmancy way of writing
>> 
>> This language doesn't fit into a commit message. Please give a technical
>> reason to change this.
>
> Oh come on!

Could you elaborate?

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

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

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

>
> 	512 ... size .. PAGE_SIZE   ------> infinity
>
> See?
> Now it is easy to insert comparisons:
>
> 	512 <= size <= PAGE_SIZE
>
> Of course in C the middle variable must be duplicated but so what?
>
> How hard is to parse this?
>
> 	512 <= size && size <= PAGE_SIZE
>
>
> And 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.

>
>> When
>> using `contains`, 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.


> This is what this patch is all about: contains() for integers?
> I can understand contains() instead of strstr() 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.

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

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


[1] https://rust.docs.kernel.org/kernel/block/mq/gen_disk/struct.GenDiskBuilder.html#method.physical_block_size