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

[Magnus Ihse Bursie]

On 2018-09-27 16:31, David Holmes wrote:

> Hi Magnus,
>
> On 27/09/2018 8:47 AM, Magnus Ihse Bursie wrote:
>> Anyone from Hotspot who can review the code changes?
>
> I had looked at this but it is unclear whether the casts are the right 
> fix or whether we have a poor type selection in the first place.  For 
> example,
>
> ./share/opto/node.hpp:  enum { NO_HASH = 0 };
>
> defines NO_HASH which is implicitly treated as int but then it's used 
> everywhere as uint - which just seems odd.
I agree; I started doing such a fix but it felt more invasive than I 
felt I could defend, so I reverted back to the simplest possible cast. 
(Which is basically what the compiler did implicitly without the warning.)
>
> So I'm unclear whether this needs more indepth examination by the 
> various code owners.
Yes, hotspotters, please do have a look. :-) Do I need to split this up 
in separate parts for different hotspot areas?

/Magnus

>
> Thanks,
> David
>
>> /Magnus
>>
>> On 2018-09-24 22:31, Magnus Ihse Bursie wrote:
>>> The -Wextra option to gcc enables a bunch of useful warnings.[1] 
>>> Some of them, but not all, can be individually enabled or disabled. 
>>> All other libraries in OpenJDK are compiled with -Wextra, but not 
>>> Hotspot. Enabling -Wextra on Hotspot triggers a couple of warnings 
>>> for zero that can be individually disabled.
>>>
>>> However, -Wextra also includes some check that cannot be disabled 
>>> individually, so to be able to add this, we must at the same time 
>>> fix those warnings.
>>>
>>> The warnings that cannot be disabled and which have been triggered 
>>> in Hotspot is "enumeral and non-enumeral type in conditional 
>>> expression" and "base class should be explicitly initialized in the 
>>> copy constructor". The former complains about mixing enums and 
>>> integers in the tertiary operator (x ? enum_val : int_val). If you 
>>> think that gcc is a bit too picky here, I agree. It's not obvious 
>>> per se that the added casts improve the code. However, this is the 
>>> price we need to pay to be able to enable -Wextra, and *that* is 
>>> something that is likely to improve the code.
>>>
>>> The second warning about the copy constructor is, for what I can 
>>> tell, a highly valid warning and the code it warned on was indeed 
>>> broken. As far as I can tell, in a derived copy constructor you 
>>> should always explicitly initialize the base class.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8211073
>>> WebRev: 
>>> http://cr.openjdk.java.net/~ihse/JDK-8211073-remove-Wno-extra-from-hotspot/webrev.01 
>>>
>>>
>>> /Magnus
>>>
>>> [1] https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
>>>
>>