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

[Thomas Schatzl]

Hi Kim,

  thanks for your review.

On Fri, 2018-08-03 at 16:41 -0400, Kim Barrett wrote:
> > On Aug 2, 2018, at 5:07 AM, Thomas Schatzl <thomas.schatzl at oracle.c
> > om> 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. 
> > 
[...]
> 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)

I knew there was some way to do it like this... thanks!

> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/g1CollectedHeap.cpp
> 1423   _expansion_regions(0),
> 
> This is an additional explicit initialization.  However, this member
> is never used.

I simply removed the member.

> -------------------------------------------------------------------
> -----------
> 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.
> 

Yes, for GC code I did that.

> -------------------------------------------------------------------
> -----------
> 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.

Fixed.

> 
> -------------------------------------------------------------------
> -----------
> 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.

You don't believe how many styles I observed in these set of patches...

I tried to improve it.

http://cr.openjdk.java.net/~tschatzl/8208669/webrev.1 (full)
http://cr.openjdk.java.net/~tschatzl/8208669/webrev.0_to_1 (diff)

Thanks,
  Thomas