Thanks for the review, Attila. Answer below. > Am 19.07.2016 um 19:04 schrieb Attila Szegedi <szege...@gmail.com>: > >> On 19 Jul 2016, at 18:20, Hannes Wallnöfer <hannes.wallnoe...@oracle.com> >> wrote: >> >> Please review: >> >> Webrev: http://cr.openjdk.java.net/~hannesw/8160034/webrev.00/ >> Bug: https://bugs.openjdk.java.net/browse/JDK-8160034 >> >> The change in ScriptObject.megamorphicGet actually fixes the bug. It took me >> quite some time that it could be fixed so elegantly at this level. Before >> that, I tried to fix it on the Method handle level in WithObject. > > So… would you say this is a solution or a workaround? What I mean is that it > is a nicely small change, but is it correct in all cases? Maybe it should be > further constrained by both isMethod and isScope? (Not sure whether there’s a > situation where it’s !isScope and would be incorrect, though) >
Excellent questions! I was convinced this is correct in all cases, and that in fact with statements are the only occurrence of such FindProperties. But your remark made me check this and indeed the same pattern occurs for global lexical declarations in Global.LexicalScope, and in that case this behavior is not desired. Although I wasn’t able to reproduce this with lexical bindings, I’ll have to do something to make sure functions are only bound when they’re meant to. Thanks, Hannes >> The cleanup in WithObject.lookup is a leftover from that. I noticed that it >> implements code paths for both named and unnamed operations, but WithObjects >> are always in scope, and scope operations are always named, so I added an >> assertion for that and removed the code handling unnamed operations. > > Yep, that part is definitely nice, +1. > >> >> Finally, the change in NashornGuards is to exclude properties from >> WithObject expressions from the special scope guards, using the ordinary map >> guard instead. Previously, these guards caused properties in with statements >> to relink even if with objects had the same map. > > +1 too. > > Attila. > >> >> Thanks, >> Hannes >
