From: Junio C Hamano <hidden> Date: 2017-01-25 21:29:05
Jeff King [off-list ref] writes:
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.
quoted hunk
-- >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 <redacted>
---
builtin/difftool.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -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;}elseif(unlink(wtdir.buf)||copy_file(wtdir.buf,rdir.buf,st.st_mode))
From: Jeff King <hidden> Date: 2017-01-25 22:01:09
On Wed, Jan 25, 2017 at 01:28:27PM -0800, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
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("").
I am OK with it either way. I was mostly responding to Dscho's
complaint, and I would just like to get this resolved so we never have
to revisit it again. :)
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.
Here's one other option that I came across. Pragmas feel gross, but I
think it will behave as we want, and it doesn't require cooperation from
the callsites at all.
-- >8 --
Subject: [PATCH] disable -Wzero-length-format via #pragma
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.
We disable this warning already with the DEVELOPER Makefile
knob. But we can't unconditionally add -Wno-format-zero-length
to the normal CFLAGS variable, because not all compilers will
understand it. So we may get reports about the warning from
non-developer users who compile with our default of -Wall.
Instead, we can disable the warning using a gcc-specific
#pragma. This should be ignored by non-gcc compilers, and do
what we want for gcc.
I tested also with clang, which often implements gcc
compatible extensions like this. Clang does not generate the
warning in the first place, but also does not complain about
our pragma.
Signed-off-by: Jeff King <redacted>
---
git-compat-util.h | 2 ++
1 file changed, 2 insertions(+)
Last time I used #pragma GCC in a cross-platform project, it triggered
an "unknown pragma" warning for MSVC. (It was the C++ compiler, I don't
know if the C compiler would also warn.) It would have to be spelled
like this:
#pragma warning(disable: 4068) /* MSVC: unknown pragma */
#pragma GCC diagnostic ignored "-Wformat-zero-length"
Dscho mentioned that he's compiling with MSVC. It would do him a favor.
-- Hannes
Last time I used #pragma GCC in a cross-platform project, it triggered
an "unknown pragma" warning for MSVC.
It is starting to become a little funny how many ways we can discuss the
resolution of the GCC compiler warning.
And it starts to show: we try to solve the thing in so many ways, just to
avoid the obviously most-trivial patch to change warning(""); to
warning("%s", "") (the change to warning(" "); would change behavior, but
I would be fine with that, too).
I am not really interested in any of these complicated workarounds. If you
gentle people decide they are better in Git's source code, go ahead. I do
not have to like what you are doing, I just have to work with it.
(It was the C++ compiler, I don't know if the C compiler would also
warn.) It would have to be spelled like this:
#pragma warning(disable: 4068) /* MSVC: unknown pragma */
#pragma GCC diagnostic ignored "-Wformat-zero-length"
Dscho mentioned that he's compiling with MSVC. It would do him a favor.
I am compiling with MSVC, and the idea is to tap into that large number of
Windows developers who Git traditionally has had a really bad time
attracting. From that perspective, I would say it would not only do me a
favor, but anybody who builds Git for Windows using Visual Studio.
But we also have to consider whether it would do anybody a "dis-favor".
#pragma statements are by definition highly dependent on the compiler. I
have no idea whether there are developers out there building Git with
C compilers other than GCC, clang or MSVC (as I did back in the days on
IRIX and HP/UX), but there is quite the potential for problems here [*1*].
To keep Git's source code truly portable, the #pragma would have to be
guarded by a GCC-specific #ifdef ... #endif.
Ciao,
Dscho
Footnote *1*: This is just another instance where a discussion on the Git
mailing list reminds me of
http://thedailywtf.com/articles/The_Complicator_0x27_s_Gloves, as it tries
to avoid an obvious solution by trying to come up with a different
solution that in turn requires additional solutions to additional problems
caused by the alternative solution.
Last time I used #pragma GCC in a cross-platform project, it triggered
an "unknown pragma" warning for MSVC.
It is starting to become a little funny how many ways we can discuss the
resolution of the GCC compiler warning.
And it starts to show: we try to solve the thing in so many ways, just to
avoid the obviously most-trivial patch to change warning(""); to
warning("%s", "") (the change to warning(" "); would change behavior, but
I would be fine with that, too).
The point is that the trivial patch fixes _this_ case, but does not
prevent the discussion from happening again later. They are two separate
problems. I am OK not solving the latter one and relying on review (as
I've already said), but the solutions do not do the same thing.
-Peff
Last time I used #pragma GCC in a cross-platform project, it triggered an
"unknown pragma" warning for MSVC. (It was the C++ compiler, I don't know if
the C compiler would also warn.) It would have to be spelled like this:
#pragma warning(disable: 4068) /* MSVC: unknown pragma */
#pragma GCC diagnostic ignored "-Wformat-zero-length"
Dscho mentioned that he's compiling with MSVC. It would do him a favor.
Bleh. The point of #pragma is to ignore ones you don't know about.
It would be easy to wrap it in an #ifdef for __GNUC__ (there is already
a similar pragma with similar wrapping in the code base).
Anyway. I do not want to make life harder for anyone. I think there are
several options floating around now, so I will let Junio decide which
one he wants to pick up.
-Peff