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

[Taylor Blau <me@ttaylorr.com>]

On Fri, May 23, 2025 at 05:12:26PM +0200, Patrick Steinhardt wrote:
> On Wed, May 21, 2025 at 02:08:40PM -0700, Junio C Hamano wrote:
> > Fernando Gouveia Lima fernandolimabusiness@gmail.com writes:
> > 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/
>
> I'm still not of the opinion that it is garbage. We have tons of
> locations where we mismatch integer types only because we never got a
> warning from the compiler, and these have caused multiple stack
> overflows in the past. The signal-to-noise ratio is high, that much is
> certainly true. But if it helps us to avoid security issues in the
> future I think that is acceptable.
>
> I do agree though this not a good project for newcomers, as fixing those
> bugs is quite intricate overall. So we should definitely remove this
> project from the microprojects page.

Agreed with the latter point.

> And in case I'm the only one who thinks that the warning has merit I'm
> also happy to be overruled and have it be removed from our developer
> settings.

Between the two of you, I tend to agree more with Junio's assessment
that the output of -Wsign-compare is generally not useful.

I don't think it's *entirely* useless, and your patches squelching its
various warnings has shown that to be the case. But having merged in
those patches to GitHub's private fork (and thus dealt with all of the
-Wsign-compare warnings that are unique to GitHub's fork), I generally
found that the warnings were not legitimate issues or downright wrong.

And indeed, merging those patches in required a fair amount of work
prior to the merge in order to get rid of those warnings. Having done
that work along with Elijah (whose perspective on this I'd be curious to
hear), I am not convinced that it made the code any better or more
secure.

I don't agree with everything in Dan Carpenter's post above there, but
I share their general sentiment. I don't feel so strongly about it
currently as to suggest we rip it out of our config.mak.dev, but I do
think it would be worth at least discussing what benefits and drawbacks
it has.

If there are a reasonable number of genuine overflow bugs or similar
that -Wsign-compare helped us find, then that's good. But if the vast
majority of the warnings are false positives, then -Wsign-compare is at
best an impediment to work around, and at worst may cause us to make a
signed-ness mistake. In that case I would feel more strongly about
removing it.

Fernando -- like Junio mentioned above, none of this is your fault
whatsoever ;-). Welcome indeed.

Thanks,
Taylor