RFR (XL): 8208669: GC changes to allow enabling -Wreorder

Kim Barrett kim.barrett at oracle.com
Fri Aug 3 20:41:36 UTC 2018


> On Aug 2, 2018, at 5:07 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi all,
> 
>  there have been several suggestions to try to fix the Hotspot code to
> allow us to enable -Wreorder in the Hotspot sources.
> This should make problems due to use-before-initialization much more
> obvious. 
> 
> This change fixes initializer lists for the GC directories.
> 
> There should be no change in code behavior, except for that there is
> iirc one minor change that fixes the memory type of one initialization
> from mtInternal to mtGC.
> 
> A later follow-up will enable -Wreorder in the makefiles.
> 
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8208669
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8208669/webrev/
> Testing:
> hs-tier1-4,jdk-tier1-3
> 
> Thanks,
>  Thomas

Looks good.

A few comments and minor things for which I don't need a new webrev.

There were some additions of explicit initialization in the
initializer list, that didn't seem associated with -Wreorder.  I'm
okay with them.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1BlockOffsetTable.cpp
  80 #ifdef ASSERT
  81   _object_can_span(false),
  82 #endif

FYI, this can also be written as
  DEBUG_ONLY(_object_can_span(false) COMMA)

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1CollectedHeap.cpp
1423   _expansion_regions(0),

This is an additional explicit initialization.  However, this member
is never used.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1CollectedHeap.cpp
1455   _evacuation_failed(false), 

This is adding an explicit initialization in the initializer list,
where there wasn't even an initialization assignment in the
constructor body. It was okay before because it is assigned in
pre_evacuate_collection_set. I'm okay with this, just confirming
intent.

------------------------------------------------------------------------------
src/hotspot/share/gc/cms/parNewGeneration.cpp
  97   _hash_seed(0),

src/hotspot/share/gc/g1/g1RegionMarkStatsCache.cpp
  36   _num_cache_entries_mask(0) {

src/hotspot/share/gc/shared/workerDataArray.inline.hpp
  35  _length(0),

Rather than initializing to a dummy value here and keeping the later
in-body assignment, move the in-body value to the initializer.

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/workgroup.cpp
 190   MutexGangTaskDispatcher()

I don't really like how this one ended up being re-formated,
especially having the (empty) constructor body way far over to the
right.  Also, aligning the colon with the constructor name seems
pretty unusual.

------------------------------------------------------------------------------



More information about the hotspot-dev mailing list