Review request for JDK-8141702: Add support for Symbol property keys

[Sundararajan Athijegannathan]

Hi,

Nice work!

Few comments:

* the symbol "internalizing" table could be per Context. Keeping it as 
static field from that class => Symbol instances are leaked permanently. 
Since we have 1:1 between script engine and nashorn Context instance, it 
is better if we keep it as Context-wide global. That way, when the 
engine dies, it's Symbols can be GC'ed.

* Classes like NativeSymbol, Symbol can be final.

* There could be comment somewhere that codegenerator will take care of 
String or Number properties in places where 'serialize' Properties, 
PropertyMaps (i.e., arbitrary Object value is taken care of).

That's it!

+1

-Sundar

On 11/10/2015 8:12 PM, Hannes Wallnoefer wrote:
> Please review JDK-8141702: Add support for Symbol property keys:
>
> http://cr.openjdk.java.net/~hannesw/8141702/
>
> This adds support for Symbols. Symbols are a new type of 
> guaranteed-unique property keys that can be used to add properties to 
> objects without risking conflicts with existing properties. Symbols 
> are used by many other features in ES6, so it helps a lot to have this 
> in.
>
> The patch looks big but for most files it's just the type of property 
> keys that changed from String to Object. This change is pretty simple, 
> and it does not affect the fast property access path.
>
> I had to jump through some hoops in order to deal with some of the 
> strange characteristics of symbols, such as the fact that implicit 
> conversion to string or number throws a TypeError. For example, I 
> introduced a dedicated toString method in tools.Shell to render 
> results so it could display symbols both as top level values and when 
> contained in an array (which neither JSType.toString nor 
> ScriptRuntime.safeToString would let us do).
>
> I think I explained all these cases sufficiently, but feel free to ask 
> if something looks weird.
>
> This change is completely invisible in ES5 mode. I also added a test 
> that checks the absence of this and other ES6 features in ES5 mode as 
> suggested by Sundar.
>
> Thanks,
> Hannes