<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"
href="https://bugs.webkit.org/show_bug.cgi?id=140855#c53">Comment # 53</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Implement ES6 arrow function syntax"
href="https://bugs.webkit.org/show_bug.cgi?id=140855">bug 140855</a>
from <span class="vcard"><a class="email" href="mailto:gskachkov@gmail.com" title="GSkachkov <gskachkov@gmail.com>"> <span class="fn">GSkachkov</span></a>
</span></b>
<pre>Comment on <span class="bz_obsolete"><a href="attachment.cgi?id=249664&action=diff" name="attach_249664" title="Patch">attachment 249664</a> <a href="attachment.cgi?id=249664&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=249664&action=review">https://bugs.webkit.org/attachment.cgi?id=249664&action=review</a>
<span class="quote">>> 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).</span >
I've changed position of the loading 'this'. I'm hope it fixed
<span class="quote">>> 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'.</span >
done
<span class="quote">>> 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.</span >
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.
<span class="quote">>> 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.</span >
I've tried to eliminate of code duplication by using DEFINE. Is it correct to use this approach in such cases?
<span class="quote">>> 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.</span >
I've merged parseArrowFunctionInfo with parseFunctionInfo and parseArrowFunctioBody with parseFunctionBody. Hope they are still readable.
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:2080
>> +#endif
>
> Nit: I think that can just be one contiguous region, not need for #endif then #if</span >
Done
<span class="quote">>> Source/JavaScriptCore/tests/stress/arrowfunction.js:1
>> +var result;
>
> I think we need some tests for lexical 'this'.</span >
I've added one more Source/JavaScriptCore/tests/stress/arrowfunction-lexicalthis.js file to test lexical 'this'.</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>