[webkit-reviews] review denied: [Bug 87755] Fix scoping bug involving eval, with, and optimized non-locals : [Attachment 144577] Patch

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has denied Andy Wingo <wingo at igalia.com>'s
request for review:
Bug 87755: Fix scoping bug involving eval, with, and optimized non-locals
https://bugs.webkit.org/show_bug.cgi?id=87755

Attachment 144577: Patch
https://bugs.webkit.org/attachment.cgi?id=144577&action=review

------- Additional Comments from Michael Saboff <msaboff at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=144577&action=review


This fix seems a little like a band-aid.  Shouldn't isDynamicScope() return
true for scope chain objects that are dynamic and require runtime resolution? 
Then the code would break out of the loop at we'd generate the right resolve
opcode.

My comment to open a new defect was incorrect.	I think we should close out
this defect as no change. We should land the reviewed patch in
https://bugs.webkit.org/show_bug.cgi?id=87158 on trunk.  A new defect should be
opened or the original refactoring defect should be reopened for your suggested
fix or variations.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1173
> +    size_t depthOfFirstScopeWithDynamicChecks = 0;

Why can't the logic for depth be modified so that it provides the right answer?


Shouldn't this variable's name really indicate the depth before the first scope
with dynamic checks?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1207
> +		   ++depthOfFirstScopeWithDynamicChecks;

If there are multiple scope chain nodes that require dynamic checks, won't this
be set to the depth before the last one?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1220
>	   if ((flags & ResolveResult::DynamicFlag) && depth)
>	       return ResolveResult::dynamicGlobalResolve(depth, scope);

Doesn't this code also need the depth before the first dynamic check?