RFR: JDK-8211073 Remove -Wno-extra from Hotspot

[Magnus Ihse Bursie]

On 2019-10-22 22:25, Kim Barrett wrote:
>> On Oct 22, 2019, at 3:17 AM, Magnus Ihse Bursie <magnus.ihse.bursie at oracle.com> wrote:
>>
>> I have tested that this compiles without warnings on gcc 4.8, 5.5, 6.5, 7.4 and 8.3 on x64. I have also tried building zero on x64, aarch64 and arm32 with gcc 8.3.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8211073
>> WebRev: http://cr.openjdk.java.net/~ihse/JDK-8211073-enable-extra-on-hotspot/webrev.01
> Change looks good.
Thanks.
>
> I remain somewhat leary of omnibus warning options in a code base like
> this, which is large and needs to support multiple compiler versions
> on multiple platforms. But at least most can be disabled if needed.
It's a three-edged sword.

I like the concept of saying to the compiler "Here's my code, give it 
your best shot to find issues with it", and then explicitly telling it 
"well, no, that's not really a problem, ignore it". And if we really had 
to specify *each and every* warnings that a compiler should check for, 
the command lines would be excruciatingly long.

On the other hand, if the set of warnings change between compiler 
versions, we have a harder time keeping the code warning free. Once 
again, if the warnings are reasonable, fixing issues is a good thing 
(and they could even have been avoided by writing good code in the first 
place :-)). But sometimes they are over-zealous and it is just an 
annoyance to fix.

But then again, even for an individual, specified warning, the 
conditions for when and how it applies can change between compiler 
versions, so the same situation is really always present, even if it's 
on a "less likely" scale.

>
> Note that gcc9 adds:
> -Wdeprecated-copy
>    Warn that the implicit declaration of a copy constructor or copy
>    assignment operator is deprecated if the class has a user-provided
>    copy constructor or copy assignment operator, in C++11 and up. This
>    warning is enabled by -Wextra. With -Wdeprecated-copy-dtor, also
>    deprecate if the class has a user-provided destructor.
>
> -Wredundant-move
>    This warning warns about redundant calls to std::move; that is, when a
>    move operation would have been performed even without the std::move
>    call.
>
> Neither of these are an issue for us yet, because we're not yet using
> C++11 or later.  But I expect -Wdeprecated-copy to cause problems for
> that update, and we might need to (temporarily) globally disable it as
> part of JEP 347.
>
> It's annoying that some of its warnings have no associated -Wno-xxx,
> in particular the one about mixing integral types and enum types in
> conditional expressions, because of the history of our code base and
> because it's perfectly well-formed usage. But apparently we don't have
> any of those right now.
>
>> On Oct 22, 2019, at 4:28 AM, Magnus Ihse Bursie <magnus.ihse.bursie at oracle.com> wrote:
>> I wouldn't count on gcc 9 bringing in anything new to -Wextra. In general, gcc is *extremely* conservative about changing -Wextra nowadays. Virtually all new warnings that are added are added as indivudual warnings with explicit names to be turned on or turned off. Only after a long vetting process does any of these gets added to -Wextra. I think the last time anything was added to -Wextra was in like gcc 6..? So the -Wextra is in a way a bit of a legacy system in gcc for handling warnings.
> This depends on what you mean by additions or changes to -Wextra. As
> mentioned above, gcc9 adds two new warnings, but they can be
> individually controlled. The set of warnings only provided by -Wextra
> and not individually controlled has indeed not changed in a long time.
> I checked, and gcc4.9 and gcc9.2 have the same set of those.
>
Yes, new warnings can be individually disabled. It's a shame that not 
all -Wextra warnings can be, but unless we're prepared to submit a patch 
to gcc, I assume we're in no position to complain. :-)

/Magnus