[Webkit-unassigned] [Bug 144956] [ES6] Implement ES6 arrow function syntax. Arrow function specific features. Lexical bind of this

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=144956

--- Comment #88 from Saam Barati <saambarati1 at gmail.com> ---
Comment on attachment 257956
  --> https://bugs.webkit.org/attachment.cgi?id=257956
Patch

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

> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:119
> +    case op_new_arrow_func_exp:

This should also report "bound this" byte code operand as a use.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:513
> +            RefPtr<RegisterID> arrowFunctionBoundThis = emitLoadArrowFunctionThis(addVar());

this should be a temporary register, not an addVar().

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2868
> +    arrowFunctionThis->ref();

don't ref this. this is an anti-pattern.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2962
> +// ------------------------------ ArrowFuncExprNode ---------------------------------

Nit: new line after comment to follow style of the file.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1744
> +            if (JSArrowFunction* function = jsDynamicCast<JSArrowFunction*>(base)) {

Will this every be nullptr when base is truthy?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3726
> +                result = weakJSConstant(function->boundThis());

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.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1273
> +            m_insertionSet.insertNode(m_indexInBlock, SpecNone, Check, node->origin,

What is this doing?
Why are we doing this instead of "fixEdge<CellUse>(node->child2())"?

> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:843
> +        case NewArrowFunction: {

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 ...)"

> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:991
> +                exactRead = FunctionActivationPLoc;

This is wrong. Revert.

> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:997
> +        case LoadArrowFunctionThis:

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.

> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:1998
> +        case NewArrowFunction: {

I think we can combine this and the below case and make necessary changes/asserts
based on if op is NewArrowFunction or NewFunction

> Source/JavaScriptCore/dfg/DFGPromoteHeapAccess.h:74
> +    case LoadArrowFunctionThis:

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).

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3074
> +    void compileNewFunction(NodeType nodeType)

We don't need NodeType as a parameter here. m_node should have all the info you need.

> Source/JavaScriptCore/jit/JIT.h:642
> +        void emitNewFuncExprCommon(Instruction*, FunctionType);

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.

> Source/JavaScriptCore/jit/JITOperations.cpp:969
> +EncodedJSValue operationNewArrowFunctionCommon(ExecState* exec, JSScope* scope, JSCell* functionExecutable, EncodedJSValue thisValue, bool isInvalidated)

I don't think you need to declare this in the header. You can just make this a static function in this file.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1016
> +inline JSFunction* getJSFunctionExpr(ExecState* exec, Instruction* pc, VM& vm)

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.

> Source/JavaScriptCore/runtime/JSArrowFunction.cpp:40
> +

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.

> Source/JavaScriptCore/runtime/JSArrowFunction.cpp:51
> +    executable->singletonFunction()->notifyWrite(vm, result, "Allocating a arrow function");

nit: "a" => "an"

> Source/JavaScriptCore/runtime/JSArrowFunction.cpp:85
> +}

nit: newline after this.

> LayoutTests/js/script-tests/arrowfunction-lexical-this-1.js:1
> +description('Tests for ES6 arrow function lexical bind of this');

I would combine "arrow function-lexical-this-1/2/3" into one file or give them more descriptive names.

> LayoutTests/js/script-tests/arrowfunction-lexical-this-2.js:9
> +shouldBe("afObjLexBind2Instance.func() === afObjLexBind2Instance", "true");

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).

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150801/21a24e01/attachment.html>