<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Implement ES6 arrow function syntax. Arrow function specific features. Lexical bind of this"
href="https://bugs.webkit.org/show_bug.cgi?id=144956#c88">Comment # 88</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Implement ES6 arrow function syntax. Arrow function specific features. Lexical bind of this"
href="https://bugs.webkit.org/show_bug.cgi?id=144956">bug 144956</a>
from <span class="vcard"><a class="email" href="mailto:saambarati1@gmail.com" title="Saam Barati <saambarati1@gmail.com>"> <span class="fn">Saam Barati</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=257956&action=diff" name="attach_257956" title="Patch">attachment 257956</a> <a href="attachment.cgi?id=257956&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=257956&action=review">https://bugs.webkit.org/attachment.cgi?id=257956&action=review</a>
<span class="quote">> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:119
> + case op_new_arrow_func_exp:</span >
This should also report "bound this" byte code operand as a use.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:513
> + RefPtr<RegisterID> arrowFunctionBoundThis = emitLoadArrowFunctionThis(addVar());</span >
this should be a temporary register, not an addVar().
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2868
> + arrowFunctionThis->ref();</span >
don't ref this. this is an anti-pattern.
<span class="quote">> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2962
> +// ------------------------------ ArrowFuncExprNode ---------------------------------</span >
Nit: new line after comment to follow style of the file.
<span class="quote">> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1744
> + if (JSArrowFunction* function = jsDynamicCast<JSArrowFunction*>(base)) {</span >
Will this every be nullptr when base is truthy?
<span class="quote">> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3726
> + result = weakJSConstant(function->boundThis());</span >
I don't think this needs to be weak. I don't think it will make any difference because the arrow function
will always keep bound this alive, but it seems more correct to me to not be weak.
If something is weak, it means that if that cell is GCed, then the resulting DFG code goes away.
If something is strong, it means the DFG code keeps that cell alive. I think strong makes more sense here.
<span class="quote">> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1273
> + m_insertionSet.insertNode(m_indexInBlock, SpecNone, Check, node->origin,</span >
What is this doing?
Why are we doing this instead of "fixEdge<CellUse>(node->child2())"?
<span class="quote">> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:843
> + case NewArrowFunction: {</span >
I would combine this case with the case below so you don't repeat code.
You can just do:
"if (op == NewArrowFunction) write.add(FunctionBoundThisPLoc ...)"
<span class="quote">> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:991
> + exactRead = FunctionActivationPLoc;</span >
This is wrong. Revert.
<span class="quote">> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:997
> + case LoadArrowFunctionThis:</span >
I don't think we need to worry about LoadArrowFunctionThis in the allocation
sinking phase. This will just be loading from callee which should be materialized already at the top of a function.
<span class="quote">> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:1998
> + case NewArrowFunction: {</span >
I think we can combine this and the below case and make necessary changes/asserts
based on if op is NewArrowFunction or NewFunction
<span class="quote">> Source/JavaScriptCore/dfg/DFGPromoteHeapAccess.h:74
> + case LoadArrowFunctionThis:</span >
I think this can be removed. It doesn't seem correct.
We're using FunctionBoundThisPLoc when sinking the arrow function, not when
actually executing an arrow function (LoadArrowFunctionThis is only used when Arrow function is the callee).
<span class="quote">> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3074
> + void compileNewFunction(NodeType nodeType)</span >
We don't need NodeType as a parameter here. m_node should have all the info you need.
<span class="quote">> Source/JavaScriptCore/jit/JIT.h:642
> + void emitNewFuncExprCommon(Instruction*, FunctionType);</span >
If you have Instruction* you don't need FunctionType. You can just look at the opcode to determine if it's an arrow function or not.
<span class="quote">> Source/JavaScriptCore/jit/JITOperations.cpp:969
> +EncodedJSValue operationNewArrowFunctionCommon(ExecState* exec, JSScope* scope, JSCell* functionExecutable, EncodedJSValue thisValue, bool isInvalidated)</span >
I don't think you need to declare this in the header. You can just make this a static function in this file.
<span class="quote">> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1016
> +inline JSFunction* getJSFunctionExpr(ExecState* exec, Instruction* pc, VM& vm)</span >
Let's get rid of this function now that it only has one caller.
It made more sense before you refactored how the arrow function object layout.
<span class="quote">> Source/JavaScriptCore/runtime/JSArrowFunction.cpp:40
> +</span >
There are some thing in this file where I'm the wrong person to review them.
I'll see if somebody else can take a look.
<span class="quote">> Source/JavaScriptCore/runtime/JSArrowFunction.cpp:51
> + executable->singletonFunction()->notifyWrite(vm, result, "Allocating a arrow function");</span >
nit: "a" => "an"
<span class="quote">> Source/JavaScriptCore/runtime/JSArrowFunction.cpp:85
> +}</span >
nit: newline after this.
<span class="quote">> LayoutTests/js/script-tests/arrowfunction-lexical-this-1.js:1
> +description('Tests for ES6 arrow function lexical bind of this');</span >
I would combine "arrow function-lexical-this-1/2/3" into one file or give them more descriptive names.
<span class="quote">> LayoutTests/js/script-tests/arrowfunction-lexical-this-2.js:9
> +shouldBe("afObjLexBind2Instance.func() === afObjLexBind2Instance", "true");</span >
Can we also try doing stuff like:
afObjLexBind2.call(V, ...)
where V might be many different things?
Basically, we should test "bound" this outside
a constructor and with different this values.
(Maybe a separate test).</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>