Re: Git compile warnings (under mac/clang)

[Jeff King <peff@peff.net>]

On Fri, Jan 23, 2015 at 12:48:29PM +0100, Johannes Schindelin wrote:

> This is what I have currently in the way of attempting to "fix" it (I
> still believe that Clang is wrong to make this a warning, and causes
> more trouble than it solves):

I agree. It is something we as the programmers cannot possibly know (the
compiler is free to decide which type however it likes) and its decision
does not impact the correctness of the code (the check is either useful
or tautological, and we cannot know which, so we are being warned about
being too careful!).

I guess you could argue that the standard defines enum-numbering to
start at 0, and increment by 1.  Therefore we should know that no valid
enum value is less than 0.  IOW, "msg_id < 0" being true must be the
result of a bug somewhere else in the program, where we assigned a value
outside of the enum range to the enum.

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

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

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

-Peff