Re: [PATCH] Makefile: suppress false positive warnings of empty format string.
On Mon, Sep 30, 2013 at 11:38:33PM +0200, Stefan Beller wrote: > > 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. > > Well actually I do think people are encouraged to play around with their > CFLAGS as much as they like. I do add link time optimisation usually. Oh, absolutely. I didn't mean to give the impression that you should not tweak CFLAGS. That's what it's there for. I just meant that I do not recall seeing complaints from people on such compilers, so either it actually works, or they are savvy enough to tweak CFLAGS without making a complaint. Or they no longer exist. The patches I'm thinking of were from 2008, and the compilers and systems were old then. > However I do have the strong opinion that any serious project should > compile without any warning/error with the standard compilers of > the current time. That's why I started an attempt again to have > -Wno-format-zero-length in there by default. Most of the people (I > assume so) are using gcc. So it should build fine there without any > warnings. Yeah, I'd agree it is a good goal. > Sure it should build without errors as well on other architectures, so > I do understand the issue to check if we're really using gcc and can > omit this flag if using another compiler. Right, agreed. > and here > http://thread.gmane.org/gmane.comp.version-control.git/230806 Thanks, I didn't recall that one. I still think if we are going to start doing gcc auto-detection in the Makefile, it is slightly less ugly to just tweak the few callsites to prevent the warning in the first place. I think gcc is being silly to warn about, but it is the path of least resistance and maintenance. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Makefile: suppress false positive warnings of empty format string.
On 09/30/2013 10:14 PM, Jeff King wrote:
> 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.
Well actually I do think people are encouraged to play around with their
CFLAGS as much as they like. I do add link time optimisation usually.
However I do have the strong opinion that any serious project should
compile without any warning/error with the standard compilers of
the current time. That's why I started an attempt again to have
-Wno-format-zero-length in there by default. Most of the people (I
assume so) are using gcc. So it should build fine there without any
warnings.
Sure it should build without errors as well on other architectures, so I
do understand the issue to check if we're really
using gcc and can omit this flag if using another compiler.
>
> 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
and here
http://thread.gmane.org/gmane.comp.version-control.git/230806
>
>> 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.
I do agree. :)
>
>> 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
>
signature.asc
Description: OpenPGP digital signature
Re: [PATCH] Makefile: suppress false positive warnings of empty format string.
On Mon, Sep 30, 2013 at 02:26:36PM -0700, Jonathan Nieder wrote: > Jeff King wrote: > > >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 > > Yeah, that looks okay (ugly but tolerable). It's tempting to do > something like > > - status_printf_ln(s, GIT_COLOR_NORMAL, ""); > + status_nl(s, GIT_COLOR_NORMAL); > > and > > - status_printf(s, color(WT_STATUS_HEADER, s), ""); > + status_start_line(s, color(WT_STATUS_HEADER, s)); > > to make the intent clearer. Sane? Yeah, I hinted at adding wrappers like this in the earlier thread but didn't actually try it. I think the approach is reasonable, as I doubt the wrappers should require too much refactoring. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Makefile: suppress false positive warnings of empty format string.
Jeff King wrote: >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 Yeah, that looks okay (ugly but tolerable). It's tempting to do something like - status_printf_ln(s, GIT_COLOR_NORMAL, ""); + status_nl(s, GIT_COLOR_NORMAL); and - status_printf(s, color(WT_STATUS_HEADER, s), ""); + status_start_line(s, color(WT_STATUS_HEADER, s)); to make the intent clearer. Sane? Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Makefile: suppress false positive warnings of empty format string.
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Makefile: suppress false positive warnings of empty format string.
Hi, Stefan Beller 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"? 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? 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. When it 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? I suspect such a documentation fix would have more impact, since it could encourage people to experiment with flags and to check their code strictly even when warnings are not portable, without significantly slowing down the normal build. For reference, todo:Make suggests the following flags: -Wpointer-arith -Woverflow -Wunused \ -Wno-pointer-to-int-cast -Werror \ -Wold-style-definition std=c99 -O2 \ -Wall -Wdeclaration-after-statement -Wno-format-zero-length -g Hope that helps, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Makefile: suppress false positive warnings of empty format string.
On 29/09/13 16:07, Felipe Contreras wrote: > On Sun, Sep 29, 2013 at 7:08 AM, Stefan Beller > wrote: >> Signed-off-by: Stefan Beller >> --- >> Makefile | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/Makefile b/Makefile >> index de3d72c..60afa51 100644 >> --- 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 > > Oh yes please. > > However, somebody mentioned that this might break compilers other than > gcc, but perhaps we can do what Linux does: I simply added: CFLAGS+=-Wno-format-zero-length to my config.mak file. I had originally intended to do so conditionally, depending on the compiler being gcc, but I found that clang and tcc just ignored it ... > cc-disable-warning = $(call try-run,\ > $(CC) $(CFLAGS) -W$(strip $(1)) -c -x c /dev/null -o "$$TMP",-Wno-$(strip > $(1))) > > CFLAGS = -g -O2 -Wall $(call cc-disable-warning,format-zero-length,) ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Makefile: suppress false positive warnings of empty format string.
On Sun, Sep 29, 2013 at 7:08 AM, Stefan Beller wrote: > Signed-off-by: Stefan Beller > --- > Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index de3d72c..60afa51 100644 > --- 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 Oh yes please. However, somebody mentioned that this might break compilers other than gcc, but perhaps we can do what Linux does: cc-disable-warning = $(call try-run,\ $(CC) $(CFLAGS) -W$(strip $(1)) -c -x c /dev/null -o "$$TMP",-Wno-$(strip $(1))) CFLAGS = -g -O2 -Wall $(call cc-disable-warning,format-zero-length,) -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Makefile: suppress false positive warnings of empty format string.
Signed-off-by: Stefan Beller --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index de3d72c..60afa51 100644 --- 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 LDFLAGS = ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) ALL_LDFLAGS = $(LDFLAGS) -- 1.8.4.474.g128a96c -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
