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

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

Fernando Gouveia Lima fernandolimabusiness@gmail.com writes:

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

> @@ -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/