Re: Git compile warnings (under mac/clang)

From: Jeff King <peff@peff.net>
Date: Jan 23, 2015, 13:30 UTC
Message-ID: <20150123133033.GA27692@peff.net>
In-Reply-To: <6fd8dc170de8be1ab38f8fda89d44f6a@www.dscho.org>

On Fri, Jan 23, 2015 at 01:38:17PM +0100, Johannes Schindelin wrote:

Show 11 quoted lines

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

Yeah, I am all in favor of defensive programming. But I am not sure that it is defending much here, as we silently fall back to an alternate value for the severity. Would we notice, or would that produce subtly wrong results? IOW, would this be better as:

  assert(msg_id >= 0 && msg_id < FSCK_MSG_MAX);

or something?

Show 17 quoted lines

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

Hmm, yeah, that does not seem unreasonable, and is more localized.

-Peff