RFR (xs) 8145940: TempNewSymbol should have correct copy and assignment functions

[John Rose]

Count me in as one who is very glad to see TempNewSymbol
getting regularized.  You can also count me as a reviewer.

Like Kim and Markus, I dislike the fact that TempNewSymbol
can decrease a refcount that it did not increase in the first place,
because new_symbol or lookup pre-incremented the refcount.
(FWIW, this has always bothered me about TempNewSymbol.)
This is the sort of convention that works best when the type
system enforces it.  Quick question:  Exactly how many of
the Symbol::* API points which yield Symbol* values include
the pre-increment?  If the answer is not "all of them" or "none
of them", we have a manually-enforced distinction which may
turn out to be fragile.

Markus, is there a tracking bug yet for addressing this?
There should be.

The "copy and swap idiom" is nice, and a new C++ trick to me.
It is a clever way of cleanly encapsulating keeping the lifecycle
invariants in the ctor/dtor pairs.  (Except for the case of TNS(S*)
which Kim, Markus, and I are uncomfortable about.)

Here's a thought, FWIW, about an incremental way forward on
the pre-increment problem.  It might be useful to split the "magic"
functions:

  TempNewSymbol lookup(int index, const char* name, int len, unsigned int hash);
  TempNewSymbol new_symbol(const char* name, TRAPS) { … }
  Symbol* lookup_increment_refcount(int index, const char* name, int len, unsigned int hash);
  Symbol* new_symbol_increment_refcount(const char* name, TRAPS) { … }

There would be an initial wave of renaming (adding _increment_refcount),
followed (perhaps) by gradual removal of the _increment_refcount versions
in favor of more consistent use of TempNewSymbol.  Arrays are tricky, alas,
so maybe the endpoint is still messy, but elsewhere we work with growable
arrays of type Handle, so maybe there is a way forward.

— John


On Jan 13, 2016, at 2:35 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
> 
> I also like Markus's suggestion of SymbolTable symbol access returning
> TempNewSymbol (perhaps under a better name!), but that is likely a
> bigger change; I think this is still an improvement.
> 
> Looks good, other than those minor things above.  Unless one of these
> leads to a substantive change, I don't need another webrev.