git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: Stefan Beller <stefanbeller@googlemail.com>
Cc: git@vger.kernel.org, gitster@pobox.com
Subject: Re: [PATCH] Makefile: suppress false positive warnings of empty format string.
Date: Sun, 29 Sep 2013 12:00:17 -0700	[thread overview]
Message-ID: <20130929190017.GA2482@elie.Belkin> (raw)
In-Reply-To: <1380456534-7582-1-git-send-email-stefanbeller@googlemail.com>

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

  parent reply	other threads:[~2013-09-29 19:00 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-07-11 23:36 What's cooking in git.git (Jul 2013, #04; Thu, 11) Junio C Hamano
2013-09-29 12:08 ` [PATCH] Makefile: suppress false positive warnings of empty format string Stefan Beller
2013-09-29 15:07   ` Felipe Contreras
2013-09-29 15:23     ` Ramsay Jones
2013-09-29 19:00   ` Jonathan Nieder [this message]
2013-09-30 20:14     ` Jeff King
2013-09-30 21:26       ` Jonathan Nieder
2013-09-30 21:34         ` Jeff King
2013-09-30 21:38       ` Stefan Beller
2013-09-30 23:23         ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130929190017.GA2482@elie.Belkin \
    --to=jrnieder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=stefanbeller@googlemail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).