Re: Git compile warnings (under mac/clang)

[Johannes Schindelin <johannes.schindelin@gmx.de>]

Hi Peff,

On 2015-01-23 13:23, Jeff King wrote:
> On Fri, Jan 23, 2015 at 12:48:29PM +0100, Johannes Schindelin wrote:
> 
>>     Pointed out by Michael Blume. Jeff King provided the pointer to a commit
>>     fixing the same issue elsewhere in the Git source code.
> 
> It may be useful to reference the exact commit (3ce3ffb8) to help people
> digging in the history (e.g., if we decide there is a better way to shut
> up this warning and we need to find all the places to undo the
> brain-damage).

Good point, thanks!

>> -	for (i = 0; i < FSCK_MSG_MAX; i++) {
>> +	for (i = FSCK_MSG_MIN + 1; i < FSCK_MSG_MAX; i++) {
> 
> Ugh. It is really a shame how covering up this warning requires
> polluting so many places. I don't think we have a better way, though,
> aside from telling people to use -Wno-tautological-compare (and I can
> believe that it _is_ a useful warning in some other circumstances, so it
> seems a shame to lose it).
> 
> Unless we are willing to drop the ">= 0" check completely. I think it is
> valid to do so regardless of the compiler's representation decision due
> to the numbering rules I mentioned above. It kind-of serves as a
> cross-check that we haven't cast some random int into the enum, but I
> think we would do better to find those callsites (since they are not
> guaranteed to work, anyway; in addition to signedness, it might choose a
> much smaller representation).

Yeah, well, this check is really more of a safety net in case I messed up anything; I was saved so many times by my own defensive programming that I try to employ it as much as I can.

But it does complicate the papering over Clang's overzealous warning, so I could live with removing the check altogether.

On the other hand, I could do something even easier:

-- snip --
diff --git a/fsck.c b/fsck.c
index 15cb8bd..8f8c82f 100644
--- a/fsck.c
+++ b/fsck.c
@@ -107,7 +107,7 @@ static int fsck_msg_severity(enum fsck_msg_id msg_id,
 {
 	int severity;
 
-	if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
+	if (options->msg_severity && ((unsigned int) msg_id) < FSCK_MSG_MAX)
 		severity = options->msg_severity[msg_id];
 	else {
 		severity = msg_id_info[msg_id].severity;
-- snap --

What do you think? Michael, does this cause more Clang warnings, or would it resolve the issue?

> I do not see either side of the bounds check here:
> 
>> +	if (options->msg_severity &&
>> +			msg_id > FSCK_MSG_MIN && msg_id < FSCK_MSG_MAX)
> 
> as really doing anything. Any code which triggers it must already cause
> undefined behavior, I think (with the exception of "msg_id == FSCK_MSG_MAX",
> but presumably that is something we never expect to happen, either).

Yep, it should not be triggered at all, but as I said, it is a nice defensive programming measure to avoid segmentation faults in case of a bug.

Ciao,
Dscho