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