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

[Volker Simonis]

On Wed, Jul 29, 2020 at 10:56 PM Liu, Xin <xxinliu at amazon.com> wrote:
>
> 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.
>

Hi Xin,

I like the new version :)

I think it's fine except the assertion in "transfer()":

 308   DirectiveSet* transfer() {
 309     assert(_origin != nullptr, "_origin is NULL! transfer() can
only be invoked once.");
 310
 311     if (_clone != nullptr) {
 312       // We are returning a (parentless) copy. The original
parent don't need to account for this.
 313       DirectivesStack::release(_origin);
 314       _origin = nullptr;
 315       return _clone;
 316     }
 317     else {
 318       return _origin;
 319     }
 320   }
 321 };

You should either move it into the " if (_clone != nullptr)" block or
set "_origin" to NULL in the "else" branch as well.

Best regards,
Volker

PS: I won't have access to mail for the next two weeks. If there won't
be any fundamental changes to this patch any more you can consider it
reviewed from my side.

> 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