Bug 212519 – [JSC] for-in should allocate new temporary register for base

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] for-in should allocate new temporary register for base

[Yusuke Suzuki]

Created attachment 400568 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/63722044>

[Yusuke Suzuki]

Created attachment 400615 [details]
Patch

[Saam Barati]

Comment on attachment 400615 [details]
Patch

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

Nice!

r=me

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1855
> +    auto canUseFastHasOwnProperty = [&] {

nice!

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1865
> +            return generator.variable(generator.propertyNames().thisIdentifier, ThisResolutionType::Local) == structureContext->baseVariable().value();

what happens for heap |this| inside arrow function?

> Source/JavaScriptCore/parser/ASTBuilder.h:1454
> +        && (dot->base()->isResolveNode() || dot->base()->isThisNode())
> +        && (!dot->base()->isResolveNode() || static_cast<ResolveNode*>(dot->base())->identifier() != "Reflect")) {

nit: you could make it:
((dot->base->isResolveNode() && ...->id() != "Reflect) || dot->base->isThisNode())
to avoid double vtable call

[Yusuke Suzuki]

Comment on attachment 400615 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1865
>> +            return generator.variable(generator.propertyNames().thisIdentifier, ThisResolutionType::Local) == structureContext->baseVariable().value();
> 
> what happens for heap |this| inside arrow function?

Arrow function loads |this| from scope to its local m_thisRegister. m_thisRegister can point to |this| in CallFrame or a variable register which is allocated for allow function.
So, after executing ensureThis(), m_thisRegister is always correct for |this|.

>> Source/JavaScriptCore/parser/ASTBuilder.h:1454
>> +        && (!dot->base()->isResolveNode() || static_cast<ResolveNode*>(dot->base())->identifier() != "Reflect")) {
> 
> nit: you could make it:
> ((dot->base->isResolveNode() && ...->id() != "Reflect) || dot->base->isThisNode())
> to avoid double vtable call

Sounds nice! Fixed.

[Yusuke Suzuki]

Created attachment 400634 [details]
Patch for landing

[Yusuke Suzuki]

Created attachment 400686 [details]
Patch for landing

[Yusuke Suzuki]

Created attachment 400687 [details]
Patch for landing

[EWS]

Committed r262354: <https://trac.webkit.org/changeset/262354>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400687 [details].