[Webkit-unassigned] [Bug 140855] [ES6] Implement ES6 arrow function syntax

[bugzilla-daemon at webkit.org]

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

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

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

Looking pretty good. Some comments below:

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1752
> +    RegisterID* value = emitThisNodeBytecode(r0, n->position());

I don't think this is doing exactly what you want here.
r0 here is the destination of this function expression, and you will 
move lexical 'this' into this destination, then, you will override that
with the function expression. I don't think this is the intention. I'm
not exactly sure of the consequences of this are, but it may be easier to
get rid of the ::emitThisNodeBytecode function, and just do this here:
```RegiserID* value = emitMove(newTemporary(), thisRegister()),``
or, you may even just be able to do:
```instructions().append(thisRegister()->index())```

> Source/JavaScriptCore/jit/JITOpcodes.cpp:985
> +    emitGetVirtualRegister(currentInstruction[4].u.operand, regT1);

I don't think you want to load 'this' after the above branch because
you need it at the branch destination. (same for 32-bit version below).

> Source/JavaScriptCore/jit/JITOpcodes.cpp:995
> +    FunctionExecutable* funcExpr = m_codeBlock->functionExpr(currentInstruction[3].u.operand);

Nit: I would call this 'executable' or 'function' or something other than 'funcExpr'.

> Source/JavaScriptCore/jit/JITOperations.cpp:951
> +    return JSValue::encode(JSBoundFunction::create(vm, globalObject, func, JSValue::decode(thisValue), boundArgs, 0, ""));

Maybe we should give this a name here instead of ""?
We should investigate this.

Also, is 0 the correct thing to pass here?
I'm not really sure what that does.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1013
> +    LLINT_RETURN(JSBoundFunction::create(vm, globalObject, func, thisValue, boundArgs, 0, ""));

ditto.

Also, maybe it's worth having some kind of helper function here 
because this function is so similar to the above.

> Source/JavaScriptCore/parser/Parser.cpp:1538
> +{

I still think this function has too much code in common with ::parseFunctionInfo,
we should abstract away the common bits.

> Source/JavaScriptCore/parser/Parser.cpp:2080
> +#endif

Nit: I think that can just be one contiguous region, not need for #endif then #if

> Source/JavaScriptCore/parser/Parser.h:560
> +    const SourceProviderCacheItem* findCachedFunctionInfo(int openBracePos)

Nit: whitespace

> Source/JavaScriptCore/parser/Parser.h:568
> +    void didFinishParsing(SourceElements*, DeclarationStacks::VarStack&,

ditto

> Source/JavaScriptCore/tests/stress/arrowfunction.js:1
> +var result;

I think we need some tests for lexical 'this'.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150328/d4f9d6a1/attachment-0002.html>