[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

Saam Barati <saambarati1 at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #258505|review?                     |review-
              Flags|                            |

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

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

Looks pretty good to me. This seems really close to being finished. Some comments below. I still need to read more closely through the tests.

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

This needs to also report the bound this operand as a use.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:83
> +UnlinkedFunctionExecutable::UnlinkedFunctionExecutable(VM* vm, Structure* structure, const SourceCode& source, RefPtr<SourceProvider>&& sourceOverride, FunctionBodyNode* node, UnlinkedFunctionKind kind, ConstructAbility constructAbility, VariableEnvironment& parentScopeTDZVariables, bool isArrowFunction)

Lets make FunctionBodyNode know about whether or not it's an arrow function instead of having an extra parameter.
(FunctionBodyNode is poorly named but it's job is to keep track of information like this).

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2301
> +    if (m_codeBlock->isConstructor())

Is there a way to do this only for ES6 classes when we're in a constructor?
Otherwise, this is probably unnecessary. I think isConstructor() will also be true in the case of "function F(){}; new F;" which isn't an ES6 constructor function.
Might be worth looking into.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2312
> +

nit: Whitespace.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1743
> +        if (JSValue base = forNode(node->child1()).m_value) {

Will it ever be the case that this is true and the below dynamic case will return nullptr?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4093
> +            else

I think we can do something even better here.
Consider this function:
"function F() { this.x = 20; var f = () => this.x; f() }"
If we inline the call to "f" into "F", then I think the get(VirtualRegister(JSStack::Callee) will result in a callee
node whose op is NewArrowFunction. If we see that callee is a NewArrowFunction, we can simply
do: "set(VirtualRegister(currentInstruction[1].u.operand), callee->child2())".

But, maybe this won't play nice with allocation sinking. It's worth looking into if doing this will report an escape with some test program that benefits from allocation sinking.
If keeping LoadArrowFunctionThis enables allocation sinking on arrow functions then it's not worth doing what I suggested.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:175
> +enum FunctionExpressionType { ArrowFunctionExpressionType, CommonFunctionExpressionType};

Is this used?

-- 
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/20150807/c889423f/attachment.html>