On Sun, Sep 29, 2013 at 12:00:17PM -0700, Jonathan Nieder wrote: > > --- a/Makefile > > +++ b/Makefile > > @@ -349,7 +349,7 @@ GIT-VERSION-FILE: FORCE > > > > # CFLAGS and LDFLAGS are for the users to override from the command line. > > > > -CFLAGS = -g -O2 -Wall > > +CFLAGS = -g -O2 -Wall -Wno-format-zero-length > > Thanks for taking this on. Two thoughts: > > 1) As Felipe mentioned, this isn't portable. Would it make sense to > make it conditional on the value of $(CC) or the output of > "$(CC) --version"? I'm not sure checking just "$(CC)" would help. Our default is "cc", which encompasses quite a wide variety of compilers, gcc and otherwise. To be honest, I'm surprised that "-Wall" doesn't create problems for older "cc" implementations. We've had patches for compiling with antique SUNWspro and MIPSpro compilers, and I sort of assumed that those don't handle "-Wall". But maybe they do. Or maybe people doing that just set CFLAGS themselves. I think the original discussion ended with "well, maybe it's not too bad for people to just turn on -Wno-format-zero-length". But if it is creating a cognitive burden on people (it's not hard to do, but you have to figure out that you need to do it), and especially if we are looking at workarounds like version-detecting gcc, I think we'd be better off to simply mark up the few callsites. Workarounds are here: http://article.gmane.org/gmane.comp.version-control.git/230026 and here: http://article.gmane.org/gmane.comp.version-control.git/230027 > 2) I don't understand the value of -Wformat-zero-length at all. What > bug is it meant to prevent? Do you know if anyone has asked the > GCC maintainers to disable it from the default set of warnings in > -Wall, which would give us a bug number to point to? I don't think there is an open bug anywhere. When this came up initially, I searched for other reports, but mostly found a handful of other people running into the same situation and adding -Wno-format-zero-length to their projects. As for the value of it, I think it is basically to detect that: printf(""); is a dead-code noop (bearing in mind that the "" may also be non-obvious when reading the code due to preprocessing), and may indicate some kind of logic error. That's reasonable to warn about; the compiler knows that stdio formatting functions with an empty format are noops. But where we run into trouble is that the warning assumes that other formatting functions are also noops in that case, which is not necessarily true. They might have side effects, or the empty string might be formatted with extra decoration (adding a newline, wrapping the empty string in quotes, etc). So I do not think the correct solution (from gcc's perspective) would be to turn off -Wformat-zero-length by default. It would rather be to enhance the annotation for the format attribute to separate the two cases, and to annotate printf() and friends with some kind of "pure-format" attribute. > 3) Since we don't enable -Werror by default (which is really good --- > use of -Werror can be a fine development tool but is a terrible > default), the warning does not actually do much harm. Yeah, I think the world is a better place if every developer of git should compiles with -Werror, but it is a terrible default for consumers of the code. > becomes harmful is when someone turns on -Werror for static > analysis purposes and is unable to move forward from there. Do we > document suggested pedantic compiler flags anywhere other than the > todo:Make script? Should we? It should probably be somewhere in the actual git.git history, as very few people look at the todo branch. I guess INSTALL or CodingGuidelines would be the most appropriate place. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html