Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Jan 26, 2017, 11:16 UTC
- Message-ID
- <alpine.DEB.2.20.1701261213060.3469@virtualbox>
- In-Reply-To
- <20170125183542.pe5qolexqqx6jhsi@sigill.intra.peff.net>
Hi Peff,
On Wed, 25 Jan 2017, Jeff King wrote:
Show 19 quoted lines
> 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 switch the DEVELOPER option on by default, when gcc or clang is used at least. Otherwise the DEVELOPER option (which I like very much) would not be able to live up to its full potential.
Another thing we should consider: paying more attention to Continuous Integration. At the moment, it happens quite frequently that `pu` builds and passes the test suite fine on Linux, but neither on Windows nor on MacOSX and it takes days to get the regressions fixed.
I vote for this patch:
Show 35 quoted lines
> -- >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.gd37c5973aCiao, Dscho