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

[Coleen Phillimore]

On 1/13/16 5:35 PM, Kim Barrett wrote:
> On Jan 13, 2016, at 3:50 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>>
>> Hi, Unfortunately, you're all getting another webrev.  I decided that I should make the test more robust, in the case where java -XX:+ExecuteInternalVMTests -version loads a class with the symbol "abc", "efg", "hij", etc. in it.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8145940.05/
> ------------------------------------------------------------------------------
> share/vm/classfile/symbolTable.hpp
>    85   // Decrement reference counter so it can go away if it's unique
>
> "unique" seems like the wrong term to me here.  Maybe "unused" or
> "unreferenced"?

Fixed. I picked "unused".
>
> ------------------------------------------------------------------------------
> src/share/vm/classfile/symbolTable.cpp
>   679   assert(ss->refcount() == abc->refcount(), "should match TempNewSymbol");
>
> I think a stronger check would be
>
>    assert(ss == abc, "...");
>
> since we have that operator==.

I'm not sure why we have operator==.   I don't remember why it was added 
or if there's code that uses it. I'd rather use operator-> which I know 
is used.
>
> ------------------------------------------------------------------------------
> src/share/vm/classfile/symbolTable.cpp
>   678   assert(ss->refcount() == abccount, "only one abc");
> ...
>   688   assert(s1->refcount() == efgcount, "one efg");
>   689   assert(s2->refcount() == hijcount, "one hij");
> &etc
>
> the "one" and "two" and such in the messages are misleading. This is
> to protect against some of these symbols already existing for other
> reasons, but I think the error messages confuse things a bit.
>
> Just a comment about the problem of pre-existing symbols, rather than
> actually changing the messages, would be sufficient for me.

Ok, I'll add a comment.  If someone is debugging the value is likely 
what's in the assert message.
>
> ------------------------------------------------------------------------------
> src/share/vm/classfile/symbolTable.cpp
>   704   s3 = SymbolTable::new_symbol("klm", CATCH);
>   705   // can't really assert that it's one
>   706   // assert(s3->refcount() == 1, "only one klm now");
>
> Capture new_symbol result in a Symbol*, record refcount, assign symbol
> to s3, and verify refcount unchanged.

ok, for some reason I thought it would be a redundant test, but it's not.
>
> ------------------------------------------------------------------------------
>
> There are other aspects of the TempNewSymbol that I might question,
> but that's for another day.
>
> 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.

Yes, further RFE.
>
> Looks good, other than those minor things above.  Unless one of these
> leads to a substantive change, I don't need another webrev.
>
Thanks, none of the changes were big.

Coleen