On 20 Jul 2016, at 12:29, Hannes Wallnöfer <hannes.wallnoe...@oracle.com> wrote: > > 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!
:-) Thanks. > 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. Frankly, I had something else in mind (I was thinking issues with functions defined in prototypes of the object used with “with”), but I re-read the relevant code for FindProperty and realized that I was mixing up the roles of “self" and “prototype” there. But if I nudged you into checking something else, great :-) As far as I’m concerned, upon further reading the FindProperty and ScriptObject code, +1 from me, unless you find that you need to adjust things for lexical scope. > 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 Cheers, Attila. > > >>> 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
