Review request for JDK-8147558: Add support for ES6 collections

[Attila Szegedi]

Global.java:
- Documentation for Global.setWeakSet() is wrong.
- It’s kinda too bad we can’t generalize those getters with lazy sentinel and builtin getters. I guess we could if they weren’t stored in fields. Sounds like a good future use case for a VarHandle :-)
- Global.initPrototype uses the “jdk.nashorn.internal.objects." string literal. It’s already used by Global.initConstructor too, maybe extract it into a constant? (FWIW, it also appears without the trailing dot in NashornLoader.java; maybe unify it into a single place somewhere?)

NativeArray.java:
- I’d refactor the common idiom in entries, keys, values, and getIterator into a separate method parametrized on iteration kind. FWIW, getIterator and values look exactly the same.

LinkedMap.java:
- why does it need to be safe for concurrent use? Seems unnecessary to me.

WeakMap and WeakSet: didn’t you need the guarantees of LinkedMap with them too? I guess not.

Other than that, +1; looks good to me. Quite a heroic effort! I’m very happy Nashorn will be offering more and more ES6 functionality.

Attila.

> On Feb 5, 2016, at 11:17 AM, Hannes Wallnöfer <hannesw at gmail.com> wrote:
> 
> Please review JDK-8147558: Add support for ES6 collections:
> 
> http://cr.openjdk.java.net/~hannesw/8147558/
> 
> The patch is rather large so here are a few things to point out:
> 
> - The Map and Set collections use a custom linked map implementation (objects/LinkedMap) instead of using a standard Java collection such as java.util.LinkedHashMap. The reason is that the forEach method in those objects allows arbitrary modifications to the base collection during iteration by the callback function. The way this is done is that additions and deletions are reflected in visited nodes except for deletion of elements that have already been visited.
> 
> See the note in http://www.ecma-international.org/ecma-262/6.0/#sec-map.prototype.foreach
> 
> LinkedMap is implemented as a linked list that uses a HashMap for hashing (or, in other words, a HashMap with linked-list nodes as values). While the hash map methods are synchronized, iteration does not use any locking. I think it is safe for concurrent use, but please do have a look.
> 
> - There are now a IS_ACCESSOR_PROPERTY flag and a isAccessorProperty() in runtime/Property. This is because ECMA6 defines several native properties as accessor properties (as opposed to data properties). In ECMA5, accessor properties always meant properties with user-provided get and set functions. The difference between data and accessor properties of course is that when accessed over the prototype chain accessor is called with the object at the start of the prototype chain as receiver instead of the  object owning the property. That's just to explain these changes. The gist of it is that it is now possible to define accessor properties through nasgen and they should behave just like user user-defined accessor properties.
> 
> - Finally, there are several small issues left in this patch that I know of, and probably more that I don't know of. I could have continued writing tests for at least another week, but I think it's good enough to push it.
> 
> Thanks!
> Hannes