git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Junio C Hamano <gitster@pobox.com>,
	David Aguilar <davvid@gmail.com>,
	Ramsay Jones <ramsay@ramsayjones.plus.com>,
	GIT Mailing-list <git@vger.kernel.org>
Subject: Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static
Date: Wed, 25 Jan 2017 13:35:42 -0500	[thread overview]
Message-ID: <20170125183542.pe5qolexqqx6jhsi@sigill.intra.peff.net> (raw)
In-Reply-To: <alpine.DEB.2.20.1701251135090.3469@virtualbox>

On Wed, Jan 25, 2017 at 11:36:50AM +0100, Johannes Schindelin wrote:

> > Gross, but at least it's self documenting. :)
> > 
> > I guess a less horrible version of that is:
> > 
> >   static inline warning_blank_line(void)
> >   {
> > 	warning("%s", "");
> >   }
> > 
> > We'd potentially need a matching one for error(), but at last it avoids
> > macro trickery.
> 
> I fail to see how this function, or this definition, makes the code better
> than simply calling `warning("%s", "");` and be done with it.

The only advantage is that it is self-documenting, so somebody does not
come through later and convert ("%s", "") back to (""). We could also
write a comment. But I am happy if we simply catch it in review (or
preferably the person is clueful enough to read the output of git-blame
and see why it is that way in the first place).

So maybe:

-- >8 --
Subject: [PATCH] difftool: hack around -Wzero-length-format warning

Building with "gcc -Wall" will complain that the format in:

  warning("")

is empty. Which is true, but the warning is over-eager. We
are calling the function for its side effect of printing
"warning:", even with an empty string.

Our DEVELOPER Makefile knob disables the warning, but not
everybody uses it. Let's silence the warning in the code so
that nobody reports it or tries to "fix" it.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/difftool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 42ad9e804..b5e85ab07 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -567,7 +567,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 				warning(_("both files modified: '%s' and '%s'."),
 					wtdir.buf, rdir.buf);
 				warning(_("working tree file has been left."));
-				warning("");
+				warning("%s", "");
 				err = 1;
 			} else if (unlink(wtdir.buf) ||
 				   copy_file(wtdir.buf, rdir.buf, st.st_mode))
-- 
2.11.0.840.gd37c5973a

  reply	other threads:[~2017-01-25 18:35 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-11-29 17:36 [PATCH] difftool.c: mark a file-local symbol with static Ramsay Jones
2016-11-30 11:07 ` Johannes Schindelin
2016-11-30 19:57   ` Ramsay Jones
2016-11-30 20:40     ` Junio C Hamano
2016-11-30 21:25       ` Jeff King
2016-11-30 22:37         ` Ramsay Jones
2016-11-30 23:18           ` Jeff King
2016-11-30 23:46             ` Junio C Hamano
2016-12-01  1:18               ` Ramsay Jones
2016-12-01  4:02                 ` Jeff King
2016-12-01 18:20                   ` Junio C Hamano
2016-12-01 18:50                     ` Jeff King
2017-01-22  5:26                       ` David Aguilar
2017-01-24 14:23                         ` Jeff King
2017-01-24 21:52                           ` Junio C Hamano
2017-01-24 23:05                             ` Jeff King
2017-01-25 10:36                               ` Fixing the warning about warning(""); was: " Johannes Schindelin
2017-01-25 18:35                                 ` Jeff King [this message]
2017-01-25 21:28                                   ` Junio C Hamano
2017-01-25 22:01                                     ` Jeff King
2017-01-26  6:39                                       ` Johannes Sixt
2017-01-26 11:37                                         ` Johannes Schindelin
2017-01-26 14:35                                           ` Jeff King
2017-01-26 14:32                                         ` Jeff King
2017-01-26 18:26                                           ` Junio C Hamano
2017-01-26 11:16                                   ` Johannes Schindelin
2017-01-26 14:39                                     ` Jeff King
2017-01-26 14:49                                       ` Johannes Schindelin

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=20170125183542.pe5qolexqqx6jhsi@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=davvid@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=ramsay@ramsayjones.plus.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).