Re: [C++] Warning Header Proposal

[Ben Longbons <brlongbons at gmail dot com>]

On Mon, Dec 16, 2013 at 7:16 AM, Pedro Alves <palves@redhat.com> wrote:
> Yes, this looks quite orthogonal to C++ to me.
I fully admit that it is possible to convert to C++ without this, and
possible to use this without converting to C++.

But, if it is not done, that means more work needs to be done by
people who are not me.

>> - I have a list of every single warning supported by gcc 4.6, 4.7, and
>> 4.8 in C++ mode (the initial version of the file in C mode will be
>> limited unless someone wants to fill it, but I don't see the point.
>
> gnulib/manywarnings.m4 also has something like that, and that's
> sure to be used by other GNU programs.  If we were to change, I'd
> rather see gnulib's warnings modules grow smart enough for our
> use (if necessary) and switch to that.
I was not aware of that file. It looks incomplete and in its present
form, it's pretty dumb; making it useful would amount to a full
rewrite. If you or someone else is willing to do that work, I have
only minor objections. (Namely, that changing a simple warning
requires rerunning autoreconf and configure, not just make)

> "warning header" vs "warnings in command line" is
> orthogonal to "decide which warnings to use based on version checks vs
> "decide which warnings to use depending on autoconf tests".  The latter
> can just as well create a warning header.
It would be possible (but kind of pointless) for autoconf control to
generate a warning header (might as well just do a @file at that
point), but it is not possible do do version-based (non-autoconf)
control without a warning header

>> Currently, gdb enables
>> very few compiler warnings. Hm, since the autoconf-added ones will be
>> in the header anyway, is there a way to tell autoconf to *not* add
>> -Wfoo to CPPFLAGS in gdb/ ?
>
> No sure what you mean.  There's -Wno-foo.

sim/, which is not going to be C++ified, would have CPPFLAGS="-Wall
-Wextra" passed from autoconf
gdb/ would have CPPFLAGS="-include gdb/warnings.hh" not
CPPFLAGS="-Wall -Wextra -include gdb/warnings.hh"

I was just thinking about shortening the command-line; this is
completely unimportant.

>> - Compared to autoconf, we can rely on warnings actually *working*
>> instead of just assuming that the option works just because the
>> compiler recognizes it.
>
> Sorry, that doesn't follow.  We absolutely can test that the warning
> actually works with autoconf.  And that's something you can't do
> with version checks.  That's the sort of thing autoconf
> is exactly meant for.  The fact that we don't do it in this particular
> case doesn't mean it can't be done.
I fully admit that autoconf, in theory, can do all this.

But there is a real difference between theory and practice. If
enabling a warning is too complicated, people just won't do it, and
gdb is the worse for it.

>> A particularly notorious example is -Wshadow,
>> which is only useful since gcc 4.8 but has been recognized since
>> (insert ridiculously ancient event here). Admittedly, it's *possible*
>> to write a test for exactly the positive and negative cases, but are
>> you really going to do that?
>
> Why not?
Do you mind actually implementing the checks for -Wshadow,
-Wmissing-declarations, and -Wredundant-decls ? All of those should
fail on at least one reasonably recent version of gcc or clang, and
may fail horribly on older versions. You have to test both for false
positives and false negatives in order to be useful.

I just don't see any way the control can be implemented in a way that
is easy to enable/disable warnings as you need them.

I think it's agreed that -Wshadow would be nice, but passing -Werror
should not break the build before it is fixed ...

...

Perhaps there is a middle ground. Autoconf could check for the
existence of a working form of warning, and #define HAVE_WSHADOW.
Then, we would have the benefit of easy toggling but also the benefits
of actually checking for the warning (provided that the test is
correct).

I'm not a huge fan of this way, because it changes the warning header
from 1 line to 3 lines per warning (ignoring comments), but it *does*
seem to meet the most essential criterion of what we each want.

Hm ... actually with macro-pasting, we could do it on one line:
WARN_IF(HAVE_WSHADOW, "-Wshadow")

I'm still not terribly fond of that (the HAVE_WFOO will not be the
same lenght, so the "-Wfoo" will not be aligned), but it's okay.

-Ben