Git compile warnings (under mac/clang)

Git compile warnings (under mac/clang)

[Michael Blume]

These are probably minor, I only bring them up because Git's build is
generally so quiet that it might be worth squashing these too.

    CC fsck.o
fsck.c:110:38: warning: comparison of unsigned enum expression >= 0 is
always true [-Wtautological-compare]
        if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
                                     ~~~~~~ ^  ~
1 warning generated.
    AR libgit.a
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib:
file: libgit.a(gettext.o) has no symbols
    CC builtin/remote.o
builtin/remote.c:1572:5: warning: add explicit braces to avoid
dangling else [-Wdangling-else]
                                else
                                ^
builtin/remote.c:1580:5: warning: add explicit braces to avoid
dangling else [-Wdangling-else]
                                else
                                ^
2 warnings generated.

(the warning about libgit.a(gettext.o) is probably because I'm
building with NO_GETTEXT -- I've never been able to get gettext to
work on my mac)

Re: Git compile warnings (under mac/clang)

[Stefan Beller]

cc Johannes Schindelin <Johannes.Schindelin@gmx.de> who is working in
the fsck at the moment
cc Peter Wu <peter@lekensteyn.nl> who worked on builtin/remote.c a few weeks ago

I just compiled origin/pu to test and also found a problem (doesn't
happen in origin/master):

http.c: In function 'get_preferred_languages':
http.c:1020:2: warning: implicit declaration of function 'setlocale'
[-Wimplicit-function-declaration]
  retval = setlocale(LC_MESSAGES, NULL);
  ^
http.c:1020:21: error: 'LC_MESSAGES' undeclared (first use in this function)
  retval = setlocale(LC_MESSAGES, NULL);
                     ^
http.c:1020:21: note: each undeclared identifier is reported only once
for each function it appears in

so I cc Yi EungJun <eungjun.yi@navercorp.com> as well.

On Thu, Jan 22, 2015 at 11:43 AM, Michael Blume <blume.mike@gmail.com> wrote:
> These are probably minor, I only bring them up because Git's build is
> generally so quiet that it might be worth squashing these too.
>
>     CC fsck.o
> fsck.c:110:38: warning: comparison of unsigned enum expression >= 0 is
> always true [-Wtautological-compare]
>         if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
>                                      ~~~~~~ ^  ~
> 1 warning generated.
>     AR libgit.a
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib:
> file: libgit.a(gettext.o) has no symbols
>     CC builtin/remote.o
> builtin/remote.c:1572:5: warning: add explicit braces to avoid
> dangling else [-Wdangling-else]
>                                 else
>                                 ^
> builtin/remote.c:1580:5: warning: add explicit braces to avoid
> dangling else [-Wdangling-else]
>                                 else
>                                 ^
> 2 warnings generated.
>
> (the warning about libgit.a(gettext.o) is probably because I'm
> building with NO_GETTEXT -- I've never been able to get gettext to
> work on my mac)
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git compile warnings (under mac/clang)

[Peter Wu]

On Thursday 22 January 2015 11:59:54 Stefan Beller wrote:
> cc Johannes Schindelin <Johannes.Schindelin@gmx.de> who is working in
> the fsck at the moment
> cc Peter Wu <peter@lekensteyn.nl> who worked on builtin/remote.c a few weeks ago
> 
> I just compiled origin/pu to test and also found a problem (doesn't
> happen in origin/master):
> 
> http.c: In function 'get_preferred_languages':
> http.c:1020:2: warning: implicit declaration of function 'setlocale'
> [-Wimplicit-function-declaration]
>   retval = setlocale(LC_MESSAGES, NULL);
>   ^
> http.c:1020:21: error: 'LC_MESSAGES' undeclared (first use in this function)
>   retval = setlocale(LC_MESSAGES, NULL);
>                      ^
> http.c:1020:21: note: each undeclared identifier is reported only once
> for each function it appears in
> 
> so I cc Yi EungJun <eungjun.yi@navercorp.com> as well.
> 
> On Thu, Jan 22, 2015 at 11:43 AM, Michael Blume <blume.mike@gmail.com> wrote:
> > These are probably minor, I only bring them up because Git's build is
> > generally so quiet that it might be worth squashing these too.
> >
> >     CC fsck.o
> > fsck.c:110:38: warning: comparison of unsigned enum expression >= 0 is
> > always true [-Wtautological-compare]
> >         if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
> >                                      ~~~~~~ ^  ~
> > 1 warning generated.
> >     AR libgit.a
> > /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib:
> > file: libgit.a(gettext.o) has no symbols
> >     CC builtin/remote.o
> > builtin/remote.c:1572:5: warning: add explicit braces to avoid
> > dangling else [-Wdangling-else]
> >                                 else
> >                                 ^
> > builtin/remote.c:1580:5: warning: add explicit braces to avoid
> > dangling else [-Wdangling-else]
> >                                 else
> >                                 ^
> > 2 warnings generated.

Hi, these warnings were present in v3 of the git-remote patch. v4 was
proposed to overcome these issues, but I have yet to respond to Junio's
feedback at http://www.spinics.net/lists/git/msg243652.html
(Message-ID: <xmqqr3vx9ad5.fsf@gitster.dls.corp.google.com>)

(cc'ing Junio to let him know I am still alive :p)

I'll get back to this next week, had some other tasks to prepare for.

Kind regards,
Peter

> > (the warning about libgit.a(gettext.o) is probably because I'm
> > building with NO_GETTEXT -- I've never been able to get gettext to
> > work on my mac)

Re: Git compile warnings (under mac/clang)

[Johannes Schindelin]

Hi Stefan,

On 2015-01-22 20:59, Stefan Beller wrote:
> cc Johannes Schindelin <Johannes.Schindelin@gmx.de> who is working in
> the fsck at the moment
>
> On Thu, Jan 22, 2015 at 11:43 AM, Michael Blume <blume.mike@gmail.com> wrote:
>
>>     CC fsck.o
>> fsck.c:110:38: warning: comparison of unsigned enum expression >= 0 is
>> always true [-Wtautological-compare]
>>         if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
>>                                      ~~~~~~ ^  ~

According to A2.5.4 of The C Programming Language 2nd edition:

    Identifiers declared as enumerators (see Par.A.8.4) are constants of type int.

Therefore, the warning is incorrect: any assumption about enum fsck_msg_id to be unsigned is false.

Ciao,
Johannes

Re: Git compile warnings (under mac/clang)

[Jeff King]

On Thu, Jan 22, 2015 at 10:20:01PM +0100, Johannes Schindelin wrote:

> On 2015-01-22 20:59, Stefan Beller wrote:
> > cc Johannes Schindelin <Johannes.Schindelin@gmx.de> who is working in
> > the fsck at the moment
> >
> > On Thu, Jan 22, 2015 at 11:43 AM, Michael Blume <blume.mike@gmail.com> wrote:
> >
> >>     CC fsck.o
> >> fsck.c:110:38: warning: comparison of unsigned enum expression >= 0 is
> >> always true [-Wtautological-compare]
> >>         if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
> >>                                      ~~~~~~ ^  ~
> 
> According to A2.5.4 of The C Programming Language 2nd edition:
> 
>     Identifiers declared as enumerators (see Par.A.8.4) are constants of type int.
> 
> Therefore, the warning is incorrect: any assumption about enum fsck_msg_id to be unsigned is false.

I'm not sure that made it to ANSI. C99 says (setion 6.7.2.2, paragraph
4):

  Each enumerated type shall be compatible with char, a signed integer
  type, or an unsigned integer type. The choice of type is
  implementation-defined, but shall be capable of representing the
  values of all the members of the enumeration.

I don't have a copy of C89, but this isn't mentioned in the (very
cursory) list of changes found in C99. Anyway, that's academic.

I think we dealt with a similar situation before, in
3ce3ffb840a1dfa7fcbafa9309fab37478605d08.

-Peff

Re: Git compile warnings (under mac/clang)

[Johannes Schindelin]

Hi Peff,

On 2015-01-22 23:01, Jeff King wrote:
> On Thu, Jan 22, 2015 at 10:20:01PM +0100, Johannes Schindelin wrote:
> 
>> On 2015-01-22 20:59, Stefan Beller wrote:
>> > cc Johannes Schindelin <Johannes.Schindelin@gmx.de> who is working in
>> > the fsck at the moment
>> >
>> > On Thu, Jan 22, 2015 at 11:43 AM, Michael Blume <blume.mike@gmail.com> wrote:
>> >
>> >>     CC fsck.o
>> >> fsck.c:110:38: warning: comparison of unsigned enum expression >= 0 is
>> >> always true [-Wtautological-compare]
>> >>         if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
>> >>                                      ~~~~~~ ^  ~
>>
>> According to A2.5.4 of The C Programming Language 2nd edition:
>>
>>     Identifiers declared as enumerators (see Par.A.8.4) are constants of type int.
>>
>> Therefore, the warning is incorrect: any assumption about enum fsck_msg_id to be unsigned is false.
> 
> I'm not sure that made it to ANSI. C99 says (setion 6.7.2.2, paragraph
> 4):
> 
>   Each enumerated type shall be compatible with char, a signed integer
>   type, or an unsigned integer type. The choice of type is
>   implementation-defined, but shall be capable of representing the
>   values of all the members of the enumeration.
> 
> I don't have a copy of C89, but this isn't mentioned in the (very
> cursory) list of changes found in C99. Anyway, that's academic.
> 
> I think we dealt with a similar situation before, in
> 3ce3ffb840a1dfa7fcbafa9309fab37478605d08.

Woooow. That commit got a chuckle out of me...

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

-- snipsnap --
commit 11b4c713f77081bf8342e5c02055ae8e18d28e8b
Author: Johannes Schindelin <johannes.schindelin@gmx.de>
Date:   Fri Jan 23 12:46:02 2015 +0100

    fsck: fix clang -Wtautological-compare with unsigned enum
    
    Clang warns that the fsck_msg_id enum is unsigned, missing that the
    specification of the C language allows for C compilers interpreting
    enums as signed.
    
    To shut up Clang, we waste a full enum value just so that we compare
    against an enum value without messing up the readability of the source
    code.
    
    Pointed out by Michael Blume. Jeff King provided the pointer to a commit
    fixing the same issue elsewhere in the Git source code.
    
    Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

diff --git a/fsck.c b/fsck.c
index 15cb8bd..f76e7a9 100644
--- a/fsck.c
+++ b/fsck.c
@@ -65,6 +65,7 @@
 
 #define MSG_ID(id, severity) FSCK_MSG_##id,
 enum fsck_msg_id {
+	FSCK_MSG_MIN = 0,
 	FOREACH_MSG_ID(MSG_ID)
 	FSCK_MSG_MAX
 };
@@ -76,6 +77,7 @@ static struct {
 	const char *id_string;
 	int severity;
 } msg_id_info[FSCK_MSG_MAX + 1] = {
+	{ NULL, -1 },
 	FOREACH_MSG_ID(MSG_ID)
 	{ NULL, -1 }
 };
@@ -85,7 +87,7 @@ static int parse_msg_id(const char *text, int len)
 {
 	int i, j;
 
-	for (i = 0; i < FSCK_MSG_MAX; i++) {
+	for (i = FSCK_MSG_MIN + 1; i < FSCK_MSG_MAX; i++) {
 		const char *key = msg_id_info[i].id_string;
 		/* id_string is upper-case, with underscores */
 		for (j = 0; j < len; j++) {
@@ -107,7 +109,8 @@ 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 &&
+			msg_id > FSCK_MSG_MIN && msg_id < FSCK_MSG_MAX)
 		severity = options->msg_severity[msg_id];
 	else {
 		severity = msg_id_info[msg_id].severity;
 

Re: Git compile warnings (under mac/clang)

[Jeff King]

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

Re: Git compile warnings (under mac/clang)

[Johannes Schindelin]

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

Re: Git compile warnings (under mac/clang)

[Jeff King]

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

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

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

Re: Git compile warnings (under mac/clang)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Re: Git compile warnings (under mac/clang)

[Jeff King]

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

Re: Git compile warnings (under mac/clang)

[Johannes Schindelin]

Hi Peff,

On 2015-01-23 19:37, Jeff King wrote:
> On Fri, Jan 23, 2015 at 10:07:18AM -0800, Junio C Hamano wrote:
> 
> [...] 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.

To be quite honest, I assumed that Git's source code was assert()-free. But I was wrong! So I'll go with that solution; it is by far the nicest one IMHO.

Thanks!
Dscho

Re: Git compile warnings (under mac/clang)

[Jeff King]

On Fri, Jan 23, 2015 at 07:46:36PM +0100, Johannes Schindelin wrote:

> > ? And then you can spell that first part as assert(), which I suspect
> > (but did not test) may shut up clang's warnings.
> 
> To be quite honest, I assumed that Git's source code was
> assert()-free. But I was wrong! So I'll go with that solution; it is
> by far the nicest one IMHO.

OK, here it is as a patch on top of js/fsck-opt. Please feel free to
squash rather than leaving it separate.

I tested with clang-3.6, and it seems to make the warning go away.

-- >8 --
Subject: [PATCH] fsck_msg_severity: range-check enum with assert()

An enum is passed into the function, which we use to index a
fixed-size array. We double-check that the enum is within
range as a defensive measure. However, there are two
problems with this:

  1. If it's not in range, we then use it to index another
     array of the same size. Which will have the same problem.
     We should probably die instead, as this condition
     should not ever happen.

  2. The bottom end of our range check is tautological
     according to clang, which generates a warning. Clang is
     not _wrong_, but the point is that we are trying to be
     defensive against something that should never happen
     (i.e., somebody abusing the enum).

We can solve both by switching to a separate assert().

Signed-off-by: Jeff King <peff@peff.net>
---
 fsck.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fsck.c b/fsck.c
index 15cb8bd..53c0849 100644
--- a/fsck.c
+++ b/fsck.c
@@ -107,7 +107,9 @@ 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)
+	assert(msg_id >= 0 && msg_id < FSCK_MSG_MAX);
+
+	if (options->msg_severity)
 		severity = options->msg_severity[msg_id];
 	else {
 		severity = msg_id_info[msg_id].severity;
-- 
2.3.0.rc1.287.g761fd19

Re: Git compile warnings (under mac/clang)

[Johannes Schindelin]

Hi Peff,

On 2015-01-23 19:55, Jeff King wrote:
> On Fri, Jan 23, 2015 at 07:46:36PM +0100, Johannes Schindelin wrote:
> 
>> > ? And then you can spell that first part as assert(), which I suspect
>> > (but did not test) may shut up clang's warnings.
>>
>> To be quite honest, I assumed that Git's source code was
>> assert()-free. But I was wrong! So I'll go with that solution; it is
>> by far the nicest one IMHO.
> 
> OK, here it is as a patch on top of js/fsck-opt. Please feel free to
> squash rather than leaving it separate.
> 
> I tested with clang-3.6, and it seems to make the warning go away.
> 
> -- >8 --
> Subject: [PATCH] fsck_msg_severity: range-check enum with assert()
> 
> An enum is passed into the function, which we use to index a
> fixed-size array. We double-check that the enum is within
> range as a defensive measure. However, there are two
> problems with this:
> 
>   1. If it's not in range, we then use it to index another
>      array of the same size. Which will have the same problem.
>      We should probably die instead, as this condition
>      should not ever happen.
> 
>   2. The bottom end of our range check is tautological
>      according to clang, which generates a warning. Clang is
>      not _wrong_, but the point is that we are trying to be
>      defensive against something that should never happen
>      (i.e., somebody abusing the enum).
> 
> We can solve both by switching to a separate assert().
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  fsck.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fsck.c b/fsck.c
> index 15cb8bd..53c0849 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -107,7 +107,9 @@ 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)
> +	assert(msg_id >= 0 && msg_id < FSCK_MSG_MAX);
> +
> +	if (options->msg_severity)
>  		severity = options->msg_severity[msg_id];
>  	else {
>  		severity = msg_id_info[msg_id].severity;

I also ended up with that solution!

Thanks!
Dscho

Re: Git compile warnings (under mac/clang)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Surely.

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

Sounds like a sensible fix to me.