Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 25, 2017, 21:28 UTC
- Message-ID
- <xmqq7f5iakxw.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170125183542.pe5qolexqqx6jhsi@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> 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).And the last sentence unfortunatly does not reflect reality.
I would prefer something self-documenting, like your wrapper with a comment. Then somebody who is looking at the implementation of warning_blank_line() will not get tempted to turn "%s", "" into "" because of the comment. And somebody who is looking at the callsite of warning_blank_line() will think twice before suggesting to turn it into warning("").
That does not make it unnecessary to review; we still need to catch those who wants to add new calls to warning("") without even knowing the presence of warning_blank_line(), if the original codepath being touched does not have any call to it.
> So maybe:
In any case, the patch is a minimum effort band-aid that lets us punt on the whole issue for now, so I'll queue it as-is.
Thanks.
Show 33 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))