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

Reply via email to