[C++] Warning Header Proposal

[C++] Warning Header Proposal

[Ben Longbons]

Under the assumption that most active gdb maintainers intend that
future gdb releases be written in C++, I am presenting a possible plan
for the conversion, based on my experience with a conversion about
1/5th the size of gdb.

I have found 3.5 areas of discussion, 2.5 of which I have actually written.
- System Requirements
- Attack Plan
-- Warning Header Proposal
- Style Guide (to be written, probably not by me)

A shared editing pad is available at http://piratepad.net/gdb-cxx, it
will contain links to the archived mails so don't be concerned about
editing what I wrote there.

This thread will discuss my proposal to make warning management easier
by using a header file.

This is a somewhat controversial proposal, so I split it from the Attack Plan.

The second step that I propose is to introduce a new mechanism for
managing warnings. This would apply only in development mode, because
for full effectiveness, it requires #pragma GCC diagnostic to apply
inside functions. This requires editing the autoconf file to add to
CPPFLAGS only under gdb/

The specific change I propose is, insteading of adding warnings by an
autoconf test, hard-code them according to compiler version. Despite
sounding like a step backward, this is actually a step forward,
because:
- 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.
Note that gcc documentation is not always accurate about which
warnings are applicable in which modes).
- Given the warning header, it very easy to flip one between "don't
care", "ignore", "force to warning even with --enable-werror",
"warning or error depending on --enable-werror", and "force error
always"
- Compared to a @file, there may be comments in a header.
- Compared to adding every single warning to the command line, this
keeps the length of the compiler line readable. 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/ ? Well, I guess we could just never add them
and pass something like: -DGDB_STRICT_WARNINGS while in gdb/ ... or
just wait for the c++ conversion and check __cplusplus.
- Compared to using autoconf, the header is much more accessible by
ordinary developers. It might not appear so to those of you who are
familiar with it, but autoconf is *insane*. I gave it a sincere
effort, but eventually I just gave up and wrote my own ./configure
script and toolset.
- Compared to autoconf, we can rely on warnings actually *working*
instead of just assuming that the option works just because the
compiler recognizes 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?

I intend to do some things differently for gdb, but for reference, the
warning file I currently use is
https://github.com/themanaworld/tmwa/blob/test/src/warnings.hpp
Particularly, I do not think it is worth trying to get meaningful
warnings out of clang.


Thanks for your time,
Ben Longbons (o11c on IRC)

Re: [C++] Warning Header Proposal

[Pedro Alves]

Hi Ben,

On 12/14/2013 12:38 AM, Ben Longbons wrote:

> This is a somewhat controversial proposal, so I split it from the Attack Plan.

Yes, this looks quite orthogonal to C++ to me.

> The specific change I propose is, insteading of adding warnings by an
> autoconf test, hard-code them according to compiler version. Despite
> sounding like a step backward, this is actually a step forward,
> because:

I read the whole email, but I fear it still sounds a step backwards
to 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.

> - Given the warning header, it very easy to flip one between "don't
> care", "ignore", "force to warning even with --enable-werror",
> "warning or error depending on --enable-werror", and "force error
> always"

"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.

> 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.

> - 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.

> 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?

-- 
Pedro Alves

Re: [C++] Warning Header Proposal

[Ben Longbons]

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