Re: [Newcomer PATCH] log-tree.c: Supress Wsign-compare-warning

From: Junio C Hamano <gitster@pobox.com>
Date: May 21, 2025, 21:08 UTC
Message-ID: <xmqqsekx8yef.fsf@gitster.g>
In-Reply-To: <20250521202409.26879-1-fernandolimabusiness@gmail.com>

Fernando Gouveia Lima fernandolimabusiness@gmail.com writes:

Show 9 quoted lines

> @@ -151,7 +151,7 @@ static int add_ref_decoration(const char *refname, const char *referent UNUSED,
>  			      int flags UNUSED,
>  			      void *cb_data)
>  {
> -	int i;
> +	long unsigned int i;
>  	struct object *obj;
>  	enum object_type objtype;
>  	enum decoration_type deco_type = DECORATION_NONE;

The complaint is about this line of code:

	for (i = 0; i < ARRAY_SIZE(ref_namespace); i++) {

where ref_namespace array is of NAMESPACE__COUNT size, which is the last enum in "enum ref_namespace". Anybody can tell it is a small integer (like 9) that would fit comfotably well within the platform natural "int" on anybody that can run Git.

ARRAY_SIZE() is defined in turns of sizeof(), so its type is "size_t"; it cannot be narrower than "int", but that is immaterial. Comparing 'i' that counts up from 0 will never cause a problem.

And the compiler ought to know all of the above, or should stay silent.

The compiler is wrong in this case. As I already have said on this list a few times, -Wsign-compare is often garbage and we should not bend over backwards to butcher perfectly good code only to silence its false positives.

Show 7 quoted lines

> @@ -458,7 +458,7 @@ void fmt_output_subject(struct strbuf *filename,
>  	}
>  	strbuf_addf(filename, "%04d-%s", nr, subject);
>  
> -	if (max_len < filename->len)
> +	if (max_len < (int) filename->len)
>  		strbuf_setlen(filename, max_len);

This conversion is wrong, even if your compiler is made silent with this change. The function begins like this:

        void fmt_output_subject(struct strbuf *filename,
                                const char *subject,
                                struct rev_info *info)
        {
                const char *suffix = info->patch_suffix;
                int nr = info->nr;
                int start_len = filename->len;
                int max_len = start_len + info->patch_name_max - (strlen(suffix) + 1);

The filename variable is a pointer to a strbuf, whose .len member is of type size_t, which can be wider than a platform natural "int". Assigning it to "int start_len" risks overflowing and wrapping around, and "int max_len" is also wrong.

So casting a large filename->len that would not fit within platform natural "int" may make your compiler silent when you compare it with max_len that is also platform natural "int". But at that point, what are you comparing with what? Imagine on a platform with 32-bit int and when the actual value of filename->len is very very long and longer than say 3 billion bytes.

In this function, I think a more reasonable thing to do is to express various length invoved in size_t, and make sure computations among them does not over/underflow.

	Side note: Those who are maintaining GSoC microproject ideas
	page.  This is the second time in a few days that I had to
	look at a patch that makes the code worse by blindly trying
	to squelch the -Wsign-compare warnings.  Can you please take
	that out of the list of suggested tasks?  Thanks.

Fernando, please do not get discouraged by _our_ code being sloppy and GSoC microproject ideas page being under-specified. Neither is your fault.

And welcome to Git development community.

[Footnote]

Quite honestly, -Wsign-compare is mostly garbage [*] and I wish we did not add it to the developer settings. A more effective way to squelch them is not by sprinkling the casts like this, but to remove it from config.mak.dev ;-)

https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/