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