Hi Attila, Thank you for the detailed review.
Regards, Srinivas -----Original Message----- From: Attila Szegedi [mailto:szege...@gmail.com] Sent: Thursday, March 02, 2017 9:01 PM To: Srinivas Dama Cc: Nashorn-dev Subject: Re: RFR: 8156743: ES6 for..of should work for Java Maps and Sets Contrary to the issue name, I don’t see anything for handling Set in the patch, but I guess that’s because Set is a Collection and those already work? As an aside… I was looking into your use of NativeJava.from to convert a Java array to a NativeArray, and realized that we have at least 3 ways to do it, each with different drawbacks: 1. NativeJava.from as used in this patch. Its drawback is that it defensively always clones the array (here we woulnd’t need to clone it, although it doesn’t hurt) 2. Global.instance.wrapAsObject. Its drawback is that its Object[] case is further down the if/else list than NativeJava.from (5th instanceof vs. 2nd) 3. Global.instance.allocate. (This one is primarily used from generated bytecode.) Its drawback is that it iterates over the elements scanning for any ScriptRuntime.EMPTY values. I guess we could pass in a boolean flag to indicate there are no empty values to skip this scan (compiler could set the flag when it can prove the array literal it’s constructing has no empties). This doesn’t affect your patch - I think NativeJava.from() is a decent choice. So, +1. Attila. > On 02 Mar 2017, at 15:36, Srinivas Dama <srinivas.d...@oracle.com> wrote: > > Hello, > > Please review http://cr.openjdk.java.net/~sdama/8156743/webrev.00/ > for https://bugs.openjdk.java.net/browse/JDK-8156743 > > Regards, > Srinivas >
