RFR: 8213481: [REDO] Fix incorrect copy constructors in hotspot
david.holmes at oracle.com
Mon Dec 3 07:33:09 UTC 2018
On 3/12/2018 5:03 pm, Kim Barrett wrote:
>> On Nov 27, 2018, at 4:34 PM, David Holmes <david.holmes at oracle.com> wrote:
>> On 28/11/2018 4:12 am, Kim Barrett wrote:
>>> The "natural result" is that the derived class copy-assign code does
>>> what it does. There are things that can go badly wrong here regarding
>>> lifetimes of embedded object references. But the asserted restriction
>>> on the allocated location of the object being copy-assigned to does
>>> nothing that I can see to prevent those potential problems.
>>> Can you provide a use-case where the assertion actually does something
>>> useful? I removed it because I don’t think such a thing exists.
>> AFAICS the only time it makes any sense to use assignment is with stack/embedded ResourceObj. The assert verifies that is the case.
>> This seems a very clear cut use of an assert to ensure an API is not being misused. Does it prevent all possible misuse - no. But that's not a reason to remove what it does do.
>> Do we ever need the assignment semantics? Maybe we should just prohibit it altogether?
> I don't understand why you might think that, and don't agree. Maybe
> there are additional unstated and unenforced restrictions?
> But part of my rejection of such a position is that a commonly
> understood idiomatic operation like assignment should either just
> work, or it shouldn't be allowed at all. Having a strange restriction
> like this makes code much harder to understand. Make a new operation
> for this restricted assignment.
How do you make a special assignment operator? AFAIK there is only one op=
ResourceObj is not a nice clean, well-defined class - it is one class
that embodies the possibility of being one of three different kinds of
"resource obj" (embedded/stack, arena-allocated, C-heap allocated) and
the operations of copying and assignment don't make sense for all of
these - hence the restriction.
> Unfortunately, there are already many uses of ResourceObj assignment.
> I spent a bit of time exploring the idea of removing or replacing it,
> but that seems way too hard for the intended scope of this cleanup.
> (Note that for some of the existing uses it is not obvious whether the
> restriction applies. Presumably it doesn't, since we're not hitting
> the assert.) While I like the idea of prohibiting assignment (and
> possibly copy too, which has many of the same issues), I'm not going
> to pursue that beyond perhaps an RFE for future work.
> BTW, as part of that exploration I found such gems as this, from
> Node_Array node_map = new Node_Array(a);
I can certainly understand people being confused about C++ allocation,
construction and assignment. So I think the above acts as:
Node_Array node_map; // Stack object using default constructor
Which means the whole "new Node_Array(a)" is completely useless (does it
cause a leak?) as operator= ignores it.
> Anyway, even though I think the assert prevents some things that would
> work (but should perhaps be avoided anyway), and doesn't prevent some
> things that won't work, I'm willing to reinstate it in order to make
> New webrevs:
> full: http://cr.openjdk.java.net/~kbarrett/8213481/open.01/
> incr: http://cr.openjdk.java.net/~kbarrett/8213481/open.01.inc/
Thanks for doing that.
More information about the hotspot-dev