Git compile warnings (under mac/clang)

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)

cc Johannes Schindelin [off-list ref] who is working in
the fsck at the moment
cc Peter Wu [off-list ref] 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 [off-list ref] as well.

On Thu, Jan 22, 2015 at 11:43 AM, Michael Blume [off-list ref] 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

On Thursday 22 January 2015 11:59:54 Stefan Beller wrote:

cc Johannes Schindelin [off-list ref] who is working in
the fsck at the moment
cc Peter Wu [off-list ref] 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 [off-list ref] as well.

On Thu, Jan 22, 2015 at 11:43 AM, Michael Blume [off-list ref] wrote:

quoted

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: [off-list ref])

(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

quoted

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

Hi Stefan,

On 2015-01-22 20:59, Stefan Beller wrote:

cc Johannes Schindelin [off-list ref] who is working in
the fsck at the moment

On Thu, Jan 22, 2015 at 11:43 AM, Michael Blume [off-list ref] wrote:

quoted

    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

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

On 2015-01-22 20:59, Stefan Beller wrote:

quoted

cc Johannes Schindelin [off-list ref] who is working in
the fsck at the moment

On Thu, Jan 22, 2015 at 11:43 AM, Michael Blume [off-list ref] wrote:

quoted

    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

Hi Peff,

On 2015-01-22 23:01, Jeff King wrote:

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

quoted

On 2015-01-22 20:59, Stefan Beller wrote:

quoted

cc Johannes Schindelin [off-list ref] who is working in
the fsck at the moment

On Thu, Jan 22, 2015 at 11:43 AM, Michael Blume [off-list ref] wrote:

quoted

    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 [off-list ref]
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 [off-list ref]

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;
 

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

Hi Peff,

On 2015-01-23 13:23, Jeff King wrote:

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

quoted

    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!

quoted

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

quoted

+	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

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

quoted

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?

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

-Peff