Re: Git compile warnings (under mac/clang)

[Jeff King <peff@peff.net>]

On Fri, Jan 23, 2015 at 10:07:18AM -0800, Junio C Hamano wrote:

> >> 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.
> 
> Or we could force enum to be signed by defining FSCK_MSG_UNUSED to
> be -1 at the very beginning of enum definition, without changing
> anything else.  Then "msg_id < 0" would become a very valid
> protection against programming mistakes, no?

Yeah, I think that would work, too. It is a little unfortunate in the
sense that it actually makes things _worse_ from the perspective of the
type system. That is, in the current code if you assume that everyone
else has followed the type rules, then an fsck_msg_id you get definitely
is indexable into various arrays. But if you add in a sentinel value,
now you (in theory) have to check for the sentinel value everywhere.

I'm not sure if that matters in practice, though, if you are going to be
defensive against people misusing the enum system in the first place
(e.g., you are worried about them passing a random int and having it
produce a segfault, you have to do range checks either way).

But of all the options outlined, I think I'd much rather just see an
assert() for something that should never happen, rather than mixing it
into the logic.

In that vein, one thing that puzzles me is that the current code looks
like:

  if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
	  severity = options->msg_severity[msg_id];
  else {
	  severity = msg_id_info[msg_id].severity;
	  ...
  }

So if the severity override list given by "options" exists, _and_ if we
are in the enum range, then we use that. Otherwise, we dereference the
global list. But wouldn't an out-of-range condition have the exact same
problem dereferencing that global list?

IOW, should this really be:

  if (msg_id < 0 || msg_id >= FSCK_MSG_MAX)
	die("BUG: broken enum");

  if (options->msg_severity)
	severity = options->msg_severity[msg_id];
  else
	severity = msg_id_info[msg_id].severity;

? And then you can spell that first part as assert(), which I suspect
(but did not test) may shut up clang's warnings.

-Peff