Thanks for the review, Attila! Am 2016-02-05 um 14:17 schrieb 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?)
Thanks, will fix the documentation and clean up the string literals.
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.
I'm not sure I understand. These are separate methods because each one defines a @Function for nasgen. Of course we could extend the @Function annotation to allow one Java method to define multiple JS functions. But these methods are one-liners anyway, so I doubt it would get much simpler.
LinkedMap.java: - why does it need to be safe for concurrent use? Seems unnecessary to me.
You're right, it's not a requirement, but I think it's nice to have given the ubiquity of multithreading on the Java platform.
WeakMap and WeakSet: didn’t you need the guarantees of LinkedMap with them too? I guess not.
The weak collections do not allow elements to be iterated over, so this isn't an issue there.
Hannes