[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 #53 from GSkachkov <gskachkov 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

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

I've changed position of the loading 'this'. I'm hope it fixed

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

done

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

Not sure that we should add the name of the function. I've checked this parameters with on following code 
var f = function (x) { return x + 1}.bind(this); and name had value "" and number of additional parameters  were 0.

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

I've tried to eliminate of code duplication by using DEFINE. Is it correct to use this approach in such cases?

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

I've merged parseArrowFunctionInfo with parseFunctionInfo and parseArrowFunctioBody with  parseFunctionBody. Hope they are still readable.

>> Source/JavaScriptCore/parser/Parser.cpp:2080
>> +#endif
> 
> Nit: I think that can just be one contiguous region, not need for #endif then #if

Done

>> Source/JavaScriptCore/tests/stress/arrowfunction.js:1
>> +var result;
> 
> I think we need some tests for lexical 'this'.

I've added one more Source/JavaScriptCore/tests/stress/arrowfunction-lexicalthis.js file to  test lexical 'this'.

-- 
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/20150401/3cc497a9/attachment.html>