On Fri, Mar 25, 2016 at 03:43:31PM +0100, Nenad Merdanovic wrote:
> Hello Willy,
>
> On 03/25/2016 03:29 PM, Nenad Merdanovic wrote:
> [..snip..]
>
> Ah, just ignore this :) I've now realized what you meant.
:-)
> Sure, I'll
> rewrite the patch like that. To me it doesn't make much difference in
> readability and they do accomplish the same purpose, so we can do it as
> you prefer.
The difference for me is that in one case I have to open the C file
to check if "i" is signed or unsigned to estimate the range of the
resulting modulo while in the other case I know that it's only applied
to a positive value so it's positive by definition. In your case the
code was protected at the next line so it was OK (and confirmed by the
two testers), but once people pick a few lines there and reuse them
somewhere else it's a different story. Or if they are simply inspired
they might not get this important detail.
So it's not a matter of strict preference, just less ambiguity or
doubt when reading a patch or a small portion of code.
Also modulos are commonly used to ensure a number is between 0 and N-1
as often used in maths, but that's clearly not how computers use them,
instead they only take the reminder of the divide operation, and it's
common when reading such code to make the wrong assumption. How many of
us have seen apparently harmless but exploitable functions like this :
char hex_digit(int v) {
return hextab[v % 16];
}
or even :
char base_digit(int v, int base) {
return base ? base36_tab[v % base] : 0;
}
That's why I tend to be extra careful where there's a risk it could be
overlooked.
Cheers,
Willy
