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

[Kim Barrett]

On Jan 7, 2016, at 3:02 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
> 
> Summary: Add clear() to the assignment operator and add copy constructor.
> 
> Ran all jtreg, colocated and non-colocated tests.   RunThese -jck with PrintSymbolTableSizeHistogram statistics:
> 
>  Percent removed          1.35
>  Reference counts          194583
> 
> clean:
> 
>  Percent removed          1.53
>  Reference counts          194245
> 
> Without a reference counting copy constructor, we could remove a TempNewSymbol's Symbol if a GC happens.  Consider:
> 
> TempNewSymbol ts = SymbolTable::new_symbol("abc");
> // Hit GC
> 
> The ref count for "abc" is 1 when created by new_symbol, and the destructor could run after the copy into ts, decrementing the reference count to 0 again.  GC could unlink that symbol from the symbol table.  Fortunately, we haven't seen this bug.
> 
> open webrev at http://cr.openjdk.java.net/~coleenp/8145940/
> bug link https://bugs.openjdk.java.net/browse/JDK-8145940
> 
> Thanks,
> Coleen

For the assignment operator, I think better is:

void operator=(TempNewSymbol s) {
  Symbol* tmp = s._temp;
  s._temp = _temp;
  _temp = tmp;
}

This is the well-known copy and swap idiom, with a by-hand "swap".

http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom

In the copy constructor, as a matter of style I would initialize _temp
in the initialer list, rather than by assignment in the body.

I think the updated description of TempNewSymbol isn't really right.
Because the conversion constructor doesn't increment the reference
count, it must *not* be used to capture some arbitrary reference to a
Symbol*. Only a new symbol or one obtained from one of the lookup
functions really should be managed by this class.

Regarding copy elision, there's a good discussion here:

http://en.cppreference.com/w/cpp/language/copy_elision