RFR (L): 8208671: Runtime, JFR, Serviceability changes to allow enabling -Wreorder

[Thomas Schatzl]

Hi David,

On Fri, 2018-08-03 at 08:45 +1000, David Holmes wrote:
> Hi Thomas,
> 
> On 2/08/2018 10:51 PM, Thomas Schatzl wrote:
> > Hi,
> > 
> > On Thu, 2018-08-02 at 22:34 +1000, David Holmes wrote:
> > > Hi Thomas,
> > > 
> > > On 2/08/2018 7:09 PM, Thomas Schatzl 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.
> > > 
> > > Can you enlighten us on what -Wreorder is/does and so what
> > > changes you are making please.
> > 
> >    oh, sorry, my fault. -Wreorder is a compiler switch that checks
> > that the order of declaration of members in the initializer lists
> > of constructors match the order of declaration in the class.
> > 
> > C++ always initializes members in the initializer list in the order
> > of the declaration in the class, it does not matter whether you
> > specify it differently in the initializer list or not.
> 
[...]
> > -Wreorder forces you to have b before a in above example in your
> > initializer list by giving a warning otherwise, which causes
> > compilation failure.
> 
> Got it. Thanks. Please add this basic info to the bug report too.

Added.

> 
> src/hotspot/share/classfile/dictionary.cpp
> 
> This looks like leftover code you used to check the order:
> 
> +
> +   static bool _some_dictionary_needs_resizing;
> +   bool _resizable;
> +   bool _needs_resizing;
> +   void check_if_needs_resize();
> +
> +   ClassLoaderData* _loader_data;  // backpointer to owning loader
> +
>    Dictionary::Dictionary(ClassLoaderData* loader_data, int
> table_size, 
> bool resizable)
> 

Thanks for catching this.

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

> 
> Otherwise it all seems okay. In some cases it may have been cleaner
> to reorder the variable declarations in the class to have related
> variables adjacent - for example following _name by _name_len, rather
> than having them split by other variables - but that's just a
> stylistic thing.

I think this should be done by the respective maintainers as cleanups
as preferred - it is beyond my knowledge to know the best order for
every class there is in hotspot, also it would have complicated this
change a lot :)

Thanks,
  Thomas