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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 21, 2025, 17:11 UTC
Message-ID: <xmqq34g7cjf2.fsf@gitster.g>
In-Reply-To: <20250221063705.15539-1-jelly.zhao.42@gmail.com>

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

Show 11 quoted lines

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