Re: [GSOC][PATCH v2 1/6] apply: change fields in `apply_state` to unsigned

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 13, 2025, 18:39 UTC
Message-ID: <xmqqikpdofjd.fsf@gitster.g>
In-Reply-To: <CAOLa=ZQbVBmGnsvWUz0ByW8z5NMgLY2oZZ1RLJeoWJST9PGAvQ@mail.gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 20 quoted lines

> Zejun Zhao <jelly.zhao.42@gmail.com> writes:
>
>> `.max_change` and `.max_len` of `apply_state` are only used as unsigned
>> integers. Misuse of `int` type would cause -Wsign-comparison warnings.
>>
>> Fix this by
>>
>>   - change `.max_change`'s type to `unsigned` since it's just a counter
>>
>
> Looking at `.max_change` it seems like this is only assigned in
> `patch_stats()` where we do
>
>   int lines = patch->lines_added + patch->lines_deleted;
>
>   if (lines > state->max_change)
>      state->max_change = lines;
>
> In this case shouldn't we first convert `.lines_added` `.lines_deleted`
> to also be 'unsigned int' in the first place?

Surely. Or if any of the internal API uses a calling convention that yields number of lines on success and negative number to signal errors, we could also unify them to signed integer instead. In either case, using types consistently is good, and thanks for sharp eyes spotting this instance.

Show 14 quoted lines

>> @@ -2257,12 +2258,12 @@ static void show_stats(struct apply_state *state, struct patch *patch)
>>  	}
>>
>>  	if (patch->is_binary) {
>> -		printf(" %-*s |  Bin\n", max, qname.buf);
>> +		printf(" %-*s |  Bin\n", (int) max, qname.buf);
>>  		strbuf_release(&qname);
>>  		return;
>>  	}
>>
>> -	printf(" %-*s |", max, qname.buf);
>> +	printf(" %-*s |", (int) max, qname.buf);
>>  	strbuf_release(&qname);
>>

This is the kind of fallout that makes the resulting code harder to read. How bad would the code churn be if we instead unify to the signed integer type, instead of using size_t, and making sure we use the range-checking versions of arithmetic when needed, I have to wonder?