On 01.09.24 21:56, Alexey Dobriyan wrote:
> On Sat, Aug 31, 2024 at 08:39:45PM +0000, Benno Lossin wrote:
>> On 31.08.24 22:15, Alexey Dobriyan wrote:
>>> 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.
>
> First, coding style mandates that there are no whitespace on both sides
> of "..". This merges all characters into one Perl-like line noise.
Can you please provide a link to that coding style? This is the default
formatting of rustfmt.
> Second, when writing C I've a habit of writing comparisons like numeric
> line in school which goes from left to right:
>
> 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
I would argue that this is just a matter of taste and familiarity. In
mathematics there are also several other ways to express the above.
For example $size \in [512, 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???
That is because the function is implemented with generics over the type
inside of the range. So if you have your own type, you can use `..` and
`..=` to create ranges. For big structs you wouldn't use by-value even
in C and C++.
>> 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.
>
> This is what this patch is all about: contains() for integers?
> I can understand contains() instead of strstr() but for integers?
Don't get me wrong, we can have a discussion about this. I just think
that it is not as clear-cut as you make it out to be.
>> 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 &.
As I said above, it's a matter of familiarity. I don't think that using
contains is inherently worse.
>>> 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.
Sure, but in this case, I don't think everything is in the function
signature.
> This is the original function:
>
> /* blk_validate_limits() validates bsize, so drivers don't usually need to */
> static inline int blk_validate_block_size(unsigned long bsize)
> {
> if (bsize < 512 || bsize > PAGE_SIZE || !is_power_of_2(bsize))
> return -EINVAL;
> return 0;
> }
>
> I have to say this is useful comment because it refers to another more
> complex function and hints that it should be used instead. It doesn't
> reiterate what the function does internally.
>
> Something was lost in translation.
We currently don't have that function in Rust. Also note that in
contrast to C, this function is private. So no driver is able to call it
anyways.
In this case, documentation is not really necessary, as we only require
it for publicly facing functions. But for those I think it is useful to
reiterate what the code does. They might be the entrypoint for first
time kernel developers and giving them an easier time of understanding
is IMO a good thing.
---
Cheers,
Benno
