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
