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

[Lois Foltan]

On 1/11/2016 6:45 PM, Kim Barrett wrote:
> 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 athttp://cr.openjdk.java.net/~coleenp/8145940/
>> bug linkhttps://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".

Kim,
I'm a bit confused, why would the assignment operator want to swap? I 
don't think s._temp should be assigned _temp's value.

> 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.

I agree with this point, can we improve the comment ahead of 
TempNewSymbol's class definition as part of this webrev?
Thanks,
Lois

> Regarding copy elision, there's a good discussion here:
>
> http://en.cppreference.com/w/cpp/language/copy_elision
>