+1, and I like the fact that the patch has become smaller. Hannes
> Am 12.12.2017 um 13:32 schrieb Priya Lakshmi Muthuswamy > <priya.lakshmi.muthusw...@oracle.com>: > > Hi, > > Kindly review. I have modified the fix to work with multiple with scopes. > > webrev : http://cr.openjdk.java.net/~pmuthuswamy/8191301/webrev.01/ > > Thanks, > Priya > On 12/5/2017 12:54 PM, Priya Lakshmi Muthuswamy wrote: >> Hi Attila, >> >> Thanks for review. >> Yes when I checked with two with scopes as suggested(JavaImporter as outer), >> current fix doesn't work. I will work on that. >> >> Thanks, >> Priya >> On 12/5/2017 12:12 PM, Attila Szegedi wrote: >>> Hm… this seems to be an issue with shared scope calls; that’s why it’s >>> sensitive to the number of similar statements. >>> >>> That said, here’s some thoughts: >>> >>> 1. Instead of >>> >>> if (self instanceof ScriptObject && >>> ((ScriptObject)self).hasWithScope()) { >>> >>> you should be able to just use >>> >>> if (self instanceof ScriptObject) { >>> >>> As then you’ll evaluate getWithScopeObject and test it for being null >>> anyway. This way, you avoid walking the prototype chain twice. >>> >>> 2. That said, I guess hasWithScope could be reimplemented simply as >>> >>> public boolean hasWithScope() { >>> return getWithScopeObject() != null; >>> } >>> >>> as both have very similar code, so it’d reduce to it nicely. (I understand >>> that you haven’t changed that, but since you were in the vicinity of that >>> code, you might as wel do it… It’s also fine if you leave it alone as it >>> is.) >>> >>> 3. One of the statements in the test is indented differently than the >>> others. >>> >>> 4. What happens if there’s _two_ with scopes, and the JavaImporter is in >>> the outer one? Does this fix still work? E.g.: >>> >>> var imports = new JavaImporter(java.lang); >>> var dummy = { x: 42, y: 13 } >>> with (imports) { >>> with (dummy) { >>> function func() { >>> System.out.println('a'); >>> System.out.println('a'); >>> System.out.println('a'); >>> System.out.println('a'); >>> System.out.println('a'); >>> System.out.println('a'); >>> System.out.println('a'); >>> }; >>> func(); >>> } >>> } >>> >>> Attila. >>> >>>> On Dec 5, 2017, at 7:13 AM, Priya Lakshmi Muthuswamy >>>> <priya.lakshmi.muthusw...@oracle.com> wrote: >>>> >>>> Hi, >>>> >>>> please review JDK-8191301 : JavaImporter fails to resolve imported >>>> elements within functions, that contain too many statements >>>> >>>> JBS : https://bugs.openjdk.java.net/browse/JDK-8191301 >>>> webrev : http://cr.openjdk.java.net/~pmuthuswamy/8191301/webrev.00/ >>>> >>>> Thanks, >>>> Priya >>>> >>>> >> >
