Re: [boost] [Wiki] Changes in information about gcc warnings.

Subject: Re: [boost] [Wiki] Changes in information about gcc warnings.
From: Patrick Horgan (phorgan1_at_[hidden])
Date: 2011-01-15 01:06:55

On 01/14/2011 06:43 AM, Philipp Reh wrote:
> Hi,
>
> I have some comments on the guidelines.
> First of all: Goob job on the article
It's a collaboration, but I've done the GCC stuff.  I can't tell you how 
excited I am to have someone pitch in with comments that 1) cares about 
it, and 2) has knowledgeable opinions.  As you'll see below I agree with 
almost everything you say, and think some of this should be added to the 
wiki page.  To do that we should come up with examples of code that 
looks correct but is not which will be exposed by each particular 
warning.  Of course better is building with these things turned on and 
then dealing with things that actually show up problems in boost.  
Examples from real boost code of problems that are shown up by building 
with warning options turned on gets people's attention.  If there's no 
bang for the buck it's a hard sale.
>
> 1) Type punning through unions is also undefined behaviour
> but all compilers I know of just allow it.
Actually, it's explicitly allowed in C.  This is documented in C99 and 
later C specs, as noted in this footnote to 6.5.2.3 Structure and union 
members:
85)If the member used to access the contents of a union object is not 
the same as the member last used to store a value in the object, the 
appropriate part of the object representation of the value is 
reinterpreted as an object representation in the new type as described 
in 6.2.6 (a process sometimes called ‘‘type punning’’). This might be a 
trap representation.

> The correct way to convert incompatible types would be to memcpy
> or std::copy (via unsigend char *) them.
Thanks.  That's certainly true for C++ and works for either C or C++, 
and applies of course to any way of moving stuff around via the use of 
char* or unsigned char*, not just memcpy or std::copy since a char* can 
alias all of memory.   I made changes to my white paper on aliasing to 
reflect that.  It's http://dbp-consulting.com/StrictAliasing.pdf and 
started originally as that section in the wiki.  I plan to bring the 
changes back, but haven't yet.  Soon.  If you want to review that as 
well, well, great!  lol!
> ... patrick did elision of stuff about VC++ which he knows nothing about;) ...
>
> 3) I would like to add some stuff to the gcc section (also clang kind of
> falls in this category as it tries to implement the warnings that gcc does
> but is still incomplete as of version 2.8).
>
> -Wall -Wextra -pedantic(-errors) is a must for gcc. There are more warnings
> that aren't turned on by that, which I consider to be useful and reveal
> bugs, and I'm going to talk about next.
I agree with that and that's what I use in my own code.  Getting 
agreement for -Wall, was like pulling teeth, there was a lot of 
resistance.  If you add to the bjam invocation:

warnings=all
you get
-Wall -pedantic
or
warnings=on
you get
-Wall
or
warning=off
you get
-w
which inhibits all warning messages.

I would like to add -Wextra to warnings=all
> * -Wconversion: It warns about truncating (or value changing) conversions
> and got kind of useful in gcc-4.3 and later. For example it will warn about
> conversions between float and int and vice versa, or narrowing conversions
> like int to short. clang is even better here and warns about signed/unsigned
> conversions as well. Sometimes this warning can reveal real bugs,
> sometimes it only forces some static_casts (but they should be used with
> great care).
Who wouldn't want to know that a conversion had truncated or changed a 
value?
> * -Wfloat-equal: Another warning that can reveal real bugs because
> comparison of floats via == or != sometimes works or it doesn't. Those
> should always be compared with an epsilon (but it is also hard to figure
> out what a large enough epsilon could be). The real problem here is that
> some template code simply cannot handle this in a reasonable way.
This one seems pretty specialized and boost code using floating point, 
done by people that understand these issues should certainly use this 
flag to avoid these easily avoided errors.
> * -Wnon-virtual-dtor: Warns about classes that have virtual functions but
> not a virtual destructor. Probably indicates a bug.
This seems obvious, yet there were boosters that argued against this in 
particular.
>
> Next are some warnings that might be useful in some cases:
>
> * -Woverloaded-virtual: Warns if a virtual function is hidden by an
> overloaded function in a derived class. Can be surpressed by typing
> "using base::foo" to bring the virtual function in scope again.
> Might also indicate a bug (like forgotten const or something).
This is a particularly crazy-making bug when you do it accidentally with 
the same signature.  It would save a lot of people hair pulling if this 
was standard to the boost warnings.
> * -Wold-style-cast: Warns about C style casts (sadly not about
> functional style casts). C style casts are only useful for
> casting "private away from base classes" because none of the C++ casts
> can do that. It is often best practice to avoid C casts altogether.
> I have seen that boost has a lot of old style casts that should be turned
> into their proper C++ counterparts instead.
That's the reason this will not be popular.  Of course this is just for 
C++.  I believe that in C++ you should always use one of `dynamic_cast', 
`static_cast', `reinterpret_cast', and `const_cast' because 1) it says 
what you mean but no more, and 2) it's less prone to side effects, and 
3) if you try to do something really crazy it won't let you and you will 
have a chance to rethink things before you shoot yourself in the foot;)
> * -Wshadow: Warns when a name hides another name (like a local variable
> hiding a member variable). Also warns about hidden typedefs in gcc-4.6.
> I think it is best practice not to do that.
I do this sometimes on purpose and wouldn't normally want to see 
warnings on it.  I would value building with this turned on occasionally 
and making sure that I'd done it on purpose.  Some names ask to be reused.
> * -Wmissing-declarations: Only available in recent gcc versions (can't
> remember which one exactly) it warns about functions that have only
> a definition but no declaration (except for inline functions, templates
> and functions in anonymous namespaces). Probably indicates a bug.
That's a good one.  It would catch when your implementation had a typo 
in the name.

Best regards,

Patrick