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

From: Junio C Hamano <gitster@pobox.com>
Date: May 23, 2025, 16:54 UTC
Message-ID: <xmqqy0unxo7d.fsf@gitster.g>
In-Reply-To: <aDCQWr3MBX4L7sbA@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

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

I know we spotted many possible overflows and wraparound in the past, but -Wsign-compare being not about sizes but signedness, I'd consider them more as happy accidents, rather than intended outcome.

If the code had 'a < (int)b' comparison where 'a' is 'int' and 'b' is 'size_t' [*], the code would still be wrong, but the compiler would not have said anything.

	[*] which is what a typical "I've suppressed the compiler
	warning that was annoying me" patch would do if the original
	were written 'a < b'.

'a -= b' can be equally bad depending on the value range of 'b', but it is not about -Wsign-compare and would go unreported, right?

So I think noise from -Wsign-compare are certainly not "false positives" (in the sense that the comparisons are between signed and unsigned---the warning option is reporting what it was asked to report), but are not-false-but-useless positives; what they try to catch is somehow different from what they could catch to help us. And that is why I have been skeptical.

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

Yeah, that is something I am quite certain about.