RFR(s): 8204958: Minor cleanups for the diagnostic framework

[Thomas Stüfe]

Hi Coleen,

On Wed, Jun 13, 2018 at 11:15 PM,  <coleen.phillimore at oracle.com> wrote:
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8204958-jcmd-cleanups/webrev.00/webrev/src/hotspot/share/services/diagnosticFramework.hpp.udiff.html
>
>  class DCmdArgIter : public ResourceObj {
>
>
> Why doesn't this constructor initialize these fields?
>
>    const char* _key_addr;
>    size_t      _key_len;
>    const char* _value_addr;
>    size_t      _value_len;
>
>

It gets initialized in DCmdArgIter::next() - assuming this is the
first method called. You are right, this should be initialized in the
constructor.

> Also since you're doing this, can you try to make the initializers in the
> constructors be in declaration order?  Someday Kim wants to turn on this
> warning that the compiler reorders the initializers that most people didn't
> realize (at least I didn't know this), and they're not in order.
>

Sure. I was somehow aware that order of init list did not matter, so
thats why I usually think it is not important.

I think I have never seen code in the hotspot where we rely on the
member initialization order, which would be bad style.

> Thank you for fixing this code and adding all the consts!
>

This kind of light grooming helps me to understand the code better.

> Coleen

I will prepare a new webrev, working in all the feedback from you and
the others.

Thanks, Thomas

>
>
>
> On 6/13/18 9:41 AM, Thomas Stüfe wrote:
>>
>> Hi all,
>>
>> while working on jcmd I saw some minor cleanup possibilities.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~stuefe/webrevs/8204958-jcmd-cleanups/webrev.00/webrev/
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8204958
>>
>> Most of them are const correctness and using initializer lists instead
>> of java-style member initialization in these C++ classes.
>>
>> Other changes:
>>
>> - Added a ResourceMark into the DCmd::parse_and_execute() loop
>> - removed DCmdFactory::create_global_DCmd() (and following that,
>> DCmdFactory::create_Cheap_instance()) since I did not find anyone
>> using that function.
>> - Also in DCmdFactory, I removed a number of setters which were never
>> called and for attributes which should be immutable: hidden, flags,
>> enabled.
>>
>> This is really a rather light cleanup. It did not touch any larger issues.
>>
>> Thank you,
>>
>> Thomas
>
>