<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#c66">Comment # 66</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:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=249968&action=diff" name="attach_249968" title="Patch">attachment 249968</a> <a href="attachment.cgi?id=249968&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=249968&action=review">https://bugs.webkit.org/attachment.cgi?id=249968&action=review</a>
I ran out of the time to review mid way but this should be a plenty of feedback to work on :)
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1768
> + instructions().append(thisRegister()->index());</span >
Are we emitting a TDZ check if an arrow function is used inside a derived class' constructor before super() is called?
e.g. new (class extends (class {}) { () => this; super(); }) should throw a ReferenceError instead of crashing.
Please add a stress test too.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1772
> +RegisterID* BytecodeGenerator::emitThisNodeBytecode(RegisterID* dst, JSTextPosition position)</span >
How is this code different at all from what's in ThisNode::emitBytecode?
I don't think we should duplicate this much code in two places.
<span class="quote">> Source/JavaScriptCore/jit/JITOperations.cpp:47
> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)
> +#include "JSArrowFunction.h"
> +#endif</span >
We should probably be checking this inside JSArrowFunction.h
<span class="quote">> Source/JavaScriptCore/jit/JITOperations.h:121
> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)
> +typedef EncodedJSValue JIT_OPERATION (*J_JITOperation_EJscCJ)(ExecState*, JSScope*, JSCell*, EncodedJSValue);
> +#endif</span >
I don't think we need to if-def op codes like this. e.g. op_check_tdz is used only when class syntax is enabled but we don't if-def it.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:598
> + bool correcArrowFinish = isEndOfArrowFunction() || match(EOFTOK);</span >
Typo: correc*t*. How is this condition different from autoSemiColon()?
It seems that we need to document the difference one way or another.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1198
> + bool handled = false;</span >
This variable is unnecessary.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1206
> + handled = true;</span >
We should just call context.setEndOffset and return here instead.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1457
> + if (parseType == ArrowFunctionParseType) {</span >
We should either use switch here or assert that parseType == ArrowFunctionParseType if the previous if evaluated to false.
Also, can we do a refactoring to put the original code into a separate helper function in a separate patch so that the diff here is readable?
As is, it's hard to review...
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1523
> + if (parseType == ArrowFunctionParseType) {
> + info.arrowFunctionBodyType = cachedInfo->isBodyArrowFunctionBlock ? ArrowFunctionBodyBlock : ArrowFunctionBodyExpression;</span >
Why do we need to condition this based on parseType?
Since arrowFunctionBodyType is never used, it should be fine to always set info.arrowFunctionBodyType.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1528
> + m_token = info.arrowFunctionBodyType == ArrowFunctionBodyBlock ? cachedInfo->closeBraceToken() : cachedInfo->endArrowFunctionToken();
> + } else
> + m_token = cachedInfo->closeBraceToken();
> +#else
> m_token = cachedInfo->closeBraceToken();</span >
Can't we just add endFunctionToken to cacheInfo instead?
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1544
> + if (parseType == ArrowFunctionParseType) {
> + if (info.arrowFunctionBodyType == ArrowFunctionBodyBlock)
> + next();
> + } else
> + next();</span >
!? Why not (parseType != ArrowFunctionParseType || info.arrowFunctionBodyType == ArrowFunctionBodyBlock)?
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1629
> + if (parseType == ArrowFunctionParseType) {
> + if (info.arrowFunctionBodyType == ArrowFunctionBodyBlock) {
> + matchOrFail(CLOSEBRACE, "Expected a closing '}' after a ", stringForFunctionMode(mode), " body");
> + next();
> + } else</span >
It looks like these two conditions can be combined into parseType == ArrowFunctionParseType && info.arrowFunctionBodyType != ArrowFunctionBodyBlock
to run the code in else since the statement in the inner if and the outer else are identical.</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>