RFR[XS] 8249809 avoid calling DirectiveSet::clone(this) in compilecommand_compatibility_init

[Liu, Xin]

hi, Volker and Tobias, 

Here is a new revision. 
http://cr.openjdk.java.net/~xliu/8249809/02/webrev/

1. This one add comments about this smart pointer and fix the formation issue. 

2. Thanks to point me out a new document of hotspot code style. 
Since it has updated to -std=c++14, I change all NULL to nullptr.

3.  I also add NON_COPYABLE because it's not intended to be copied. 


DirectiveSetPtr is just a thin wrapper of the raw pointer. if users only use it to read,  nothing will be cloned. It simply goes through. 

thanks, 
--lx

________________________________________
From: Volker Simonis <volker.simonis at gmail.com>
Sent: Wednesday, July 29, 2020 7:34 AM
To: Tobias Hartmann
Cc: Liu, Xin; hotspot-compiler-dev at openjdk.java.net
Subject: RE: [EXTERNAL] RFR[XS] 8249809 avoid calling DirectiveSet::clone(this) in compilecommand_compatibility_init

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.



On Wed, Jul 29, 2020 at 9:38 AM Tobias Hartmann
<tobias.hartmann at oracle.com> wrote:
>
> Hi Xin,
>
> On 28.07.20 22:56, Liu, Xin wrote:
> > http://cr.openjdk.java.net/~xliu/8249809/01/webrev/
>
> Overall looks good to me.
>
> Some style comments:
> - Add a comment to 'DirectiveSetPtr' to describe its purpose
> - Why not put the "cloned" logic in "operator->"?

Because there's also a "read-only" access  of the DirectiveSetPtr
which doesn't mutate its content and therefore should clone the
underlying DirectiveSet. See my first mail where I proposed to add a
second, `const`-version of "operator->". But that still required const
casts in the places where we didn't want to clone. I've therefore
voted for the new "cloned()" method which makes cloning and mutating
explicit and which is much easier to understand from my point of view
(compared to two overloaded operators).

> - Do not use the _clone pointer as boolean (see "Miscellaneous" section in the style guide [1])
> - Indentation in line 301-303 is wrong
> - Line 306 use brackets around the "else" and move it one line up "} else {"
>
> Best regards,
> Tobias
>
> [1] https://hg.openjdk.java.net/jdk/jdk/raw-file/tip/doc/hotspot-style.html