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!
> > 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.
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
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???
> 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?
> 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 &.
> > 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.
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.
