[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 #70 from GSkachkov <gskachkov at gmail.com> ---
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

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

I've added test Source/JavaScriptCore/tests/stress/arrowfunction-tdz.js.

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

Removed duplication.

>> Source/JavaScriptCore/jit/JITOperations.cpp:47
>> +#endif
> 
> We should probably be checking this inside JSArrowFunction.h

Done.

>> Source/JavaScriptCore/jit/JITOperations.h:121
>> +#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.

removed if-def

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

I'll take a look deeper in autoSemiColon(), possible this functions both do the same.

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

Done

>> Source/JavaScriptCore/parser/Parser.cpp:1206
>> +            handled = true;
> 
> We should just call context.setEndOffset and return here instead.

Done

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

I'll try to refactor to increase patch readability this weekend.

>> Source/JavaScriptCore/parser/Parser.cpp:1528
>>          m_token = cachedInfo->closeBraceToken();
> 
> Can't we just add endFunctionToken to cacheInfo instead?

Introduced new function with name endFunctionToken instead of closeBraceToken() & endArrowFunctionToken

>> Source/JavaScriptCore/parser/Parser.cpp:1544
>> +            next();
> 
> !?  Why not (parseType != ArrowFunctionParseType || info.arrowFunctionBodyType == ArrowFunctionBodyBlock)?

Fixed

>> Source/JavaScriptCore/parser/Parser.cpp:1629
>> +        } 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.

Done

-- 
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/20150409/61096b35/attachment.html>