[webkit-reviews] review granted: [Bug 226677] [JSC] Use LocalClosureVar to get brand from scope : [Attachment 430679] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 226677: [JSC] Use LocalClosureVar to get brand from scope
https://bugs.webkit.org/show_bug.cgi?id=226677

Attachment 430679: Patch

https://bugs.webkit.org/attachment.cgi?id=430679&action=review




--- Comment #6 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 430679
  --> https://bugs.webkit.org/attachment.cgi?id=430679
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430679&action=review

r=me

> Source/JavaScriptCore/ChangeLog:9
> +	   Private brand lookup is doing wrong way to get scope.

wording suggestion: "Private brand's scope lookup is using the wrong scope:"

> Source/JavaScriptCore/ChangeLog:36
> +	   Only allowed case for the above usage is LocalClosureVar. And
generatorification uses it too. In this patch,

I think this architecturally is fine. But "LocalClosureVar" now feels like the
wrong name. Maybe let's switch it to "ResolvedClosureVar", since that's
semantically how we're using it now.

It might also not need a scope depth passed to it anymore?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2836
> +	   localScopeDepth(),

do we even need this?