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