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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 18, 2025, 17:49 UTC
Message-ID: <xmqqpljfxhvp.fsf@gitster.g>
In-Reply-To: <20250216071202.61372-1-jelly.zhao.42@gmail.com>

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

Show 18 quoted lines

> On Thu, Feb 13 2025 07:08:13 +0100, Patrick Steinhardt wrote,
>> > @@ -2706,7 +2707,7 @@ static int find_pos(struct apply_state *state,
>> >  {
>> >  	int i;
>> >  	unsigned long backwards, forwards, current;
>> > -	int backwards_lno, forwards_lno, current_lno;
>> > +	size_t backwards_lno, forwards_lno, current_lno;
>> >  
>> >  	/*
>> >  	 * When running with --allow-overlap, it is possible that a hunk is
>> 
>> These are a bit curious, as they store `line`, which is itself an `int`
>> parameter. As far as I understand, the only caller is also only ever
>> passing a positive integer here.
>
> They do store an `int` parameter, which is `line`. However, we cannot change 
> `line` to unsigned since it can temporarily store an negative value before 
> the assignments to these `*_lno` happen.

Correct. 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, but then shouldn't we fix these places, not the types of these line-number variables that use the platform natural integers?

THanks.