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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Andreas,

On Mon, Sep 02, 2024 at 09:57:17AM +0000, Andreas Hindborg wrote:
> 
> 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.

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.

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

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.

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

This is a private function and an implementation detail. Why does it
need to be exposed in documentation at all?

Thanks.

-- 
Dmitry