[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 #66 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 249968
  --> https://bugs.webkit.org/attachment.cgi?id=249968
Patch

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

I ran out of the time to review mid way but this should be a plenty of feedback to work on :)

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1768
> +    instructions().append(thisRegister()->index());

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.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1772
> +RegisterID* BytecodeGenerator::emitThisNodeBytecode(RegisterID* dst, JSTextPosition position)

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.

> Source/JavaScriptCore/jit/JITOperations.cpp:47
> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)
> +#include "JSArrowFunction.h"
> +#endif

We should probably be checking this inside JSArrowFunction.h

> Source/JavaScriptCore/jit/JITOperations.h:121
> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)
> +typedef EncodedJSValue JIT_OPERATION (*J_JITOperation_EJscCJ)(ExecState*, JSScope*, JSCell*, EncodedJSValue);
> +#endif

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.

> Source/JavaScriptCore/parser/Parser.cpp:598
> +    bool correcArrowFinish = isEndOfArrowFunction() || match(EOFTOK);

Typo: correc*t*.  How is this condition different from autoSemiColon()?
It seems that we need to document the difference one way or another.

> Source/JavaScriptCore/parser/Parser.cpp:1198
> +    bool handled = false;

This variable is unnecessary.

> Source/JavaScriptCore/parser/Parser.cpp:1206
> +            handled = true;

We should just call context.setEndOffset and return here instead.

> Source/JavaScriptCore/parser/Parser.cpp:1457
> +    if (parseType == ArrowFunctionParseType) {

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

> Source/JavaScriptCore/parser/Parser.cpp:1523
> +        if (parseType == ArrowFunctionParseType) {
> +            info.arrowFunctionBodyType = cachedInfo->isBodyArrowFunctionBlock ? ArrowFunctionBodyBlock : ArrowFunctionBodyExpression;

Why do we need to condition this based on parseType?
Since arrowFunctionBodyType is never used, it should be fine to always set info.arrowFunctionBodyType.

> 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();

Can't we just add endFunctionToken to cacheInfo instead?

> Source/JavaScriptCore/parser/Parser.cpp:1544
> +        if (parseType == ArrowFunctionParseType) {
> +            if (info.arrowFunctionBodyType == ArrowFunctionBodyBlock)
> +                next();
> +        } else
> +            next();

!?  Why not (parseType != ArrowFunctionParseType || info.arrowFunctionBodyType == ArrowFunctionBodyBlock)?

> 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

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.

-- 
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/20150402/e3f27622/attachment.html>