Re: [GSOC][PATCH] apply: address -Wsign-comparison warnings

[Junio C Hamano <gitster@pobox.com>]

Zejun Zhao <jelly.zhao.42@gmail.com> writes:

> On Tue, Feb 18 2025 09:49:46 -0800, Junio C Hamano wrote,
>> Doesn't it mean that a change that makes these line-number
>> variables to size_t is wrong?  Of course the change is not made to
>> break the code but may be to please some code paths in other parts
>> of the system that wants these line-number variables that are
>> currently "int" to compare or assign without range-checking with
>> "size_t" quantity or variable,
>
> Sorry but I don't think the change here is wrong. These line-number variables 
> are used as unsigned integers (compared with unsigned, used to index arrays, 
> ...).

I thought that we made it "int" not "unsigned" as functions that
compute these line numbers needed to signal an error "oh the input
is not a valid line number" and one of the most natural ways in C to
do so was to return a negative values from them.  Isn't that what is
happening?

For that kind of use, the storage (i.e. variables and structure
members) does not have to be signed as long as we check the error
returns from these functions (and I think we do), but then compilers
may give you needress warnings when you do assign such a value
returned from the function to the final storage after checking the
validity of the value.  You may use casts but that is trading clean
code for clean compilation without complaint from needless warnings.

Or you can make the storage also signed and we do not have to worry
about squelching the warnings with ugly casts.  When we know we are
not handling patches with a billion of lines, losing half the range
by going signed is rather a no-brainer choice between the two.

I do not see any good justification, even if we buy "the final
storage should be unsigned, since we always check before assignment"
(which I am not too opposed to), for using size_t instead of
platform natural integer (be it signed or unsigned) for the line
numbers, though.

Thanks.