[Webkit-unassigned] [Bug 144955] [ES6] Implement ES6 arrow function syntax. Parser of arrow function with execution as common function

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=144955

--- Comment #58 from GSkachkov <gskachkov at gmail.com> ---
Comment on attachment 254445
  --> https://bugs.webkit.org/attachment.cgi?id=254445
Patch

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

>> Source/JavaScriptCore/parser/Parser.cpp:607
>> +    bool correctArrowFinish = isEndOfArrowFunction() || match(EOFTOK);
> 
> `match(EOFTOK)` is contained in `isEndOfArrowFunction()`. So I think `match(EOFTOK)` is not needed. Correct?
> If so, we can change it to `failIfFalse(isEndOfArrowFunction(), "...");` by merging the following check.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:608
>> +    failIfFalse(correctArrowFinish, "Expected a ';', ']', '}', ')', or ',' following a arrow function statement");
> 
> it seems refering to EOF and Line terminator is needed.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:1257
>> +        result = parseArrowFunctionStatement(context, parseType);
> 
> This `parseArrowFunctionStatement` name confuses me.
> This function is used when
> 
> (e) `=> expr;`
> 
> part, right? If so, I think clearer name is encouraged.

I've renamed it to parseArrowFunctionExpression.

>> Source/JavaScriptCore/parser/Parser.cpp:1407
>> +template <typename LexerType> template <class TreeBuilder> int Parser<LexerType>::parseArrowFunctionParameters(TreeBuilder& context, FunctionParseMode mode, ParserFunctionInfo<TreeBuilder>& info)
> 
> Why parseArrowFunctionParameters is needed in addition to `parseFunctionParameters`?
> Is it possible just using `parseFunctionParameters`?

Yep, I've refactored, please take a look if it looks better.

>> Source/JavaScriptCore/parser/Parser.cpp:1585
>> +        position.offset++;
> 
> Is it handled when <CR><LF> comes? Hm, this part looks a little bit weird.

It set position to <CR>, because there are no TOCKEN NextLine. So we back to last before new line and add move one position forward

>> Source/JavaScriptCore/parser/Parser.cpp:1621
>> +        failIfFalse(isEndOfArrowFunction(), "Expected the closing ';' ',' ']' ')' '}' after arrow function");
> 
> it seems refering to EOF and Line terminator is needed.

Done

>> Source/JavaScriptCore/parser/Parser.h:632
>> +        
> 
> Is the above match(EOFTOK) check needed?

Yes. 
It solve case when you at EOF and uses save point. If you do restoreSavePoint(saveArrowFunctionPoint) it will back to one step before EOF

>> Source/JavaScriptCore/parser/SourceCode.h:130
>> +            UChar symbol = provider()->source()[endArrowFunction - 1];
> 
> I suggest adding `ASSERT(endArrowFunction >= 1);` if it's guaranteed.

Done

>> Source/JavaScriptCore/parser/SourceCode.h:131
>> +            ASSERT_UNUSED(symbol, symbol == 13 || symbol == 10 || symbol == ',' || symbol == ';' || symbol == ')' || symbol == ']' || symbol == '}');
> 
> What are 13 and 10? Is it possible to use Lexer<...>::isLineTerminator? So are 0x2028/0x2029 is handled? (They are also line terminators.)

I've added new function isLineTerminator. I'm not sure that it will be good idea to add new dependany to this module. As for me Lexer is from another layer, and SourceCode should not interact with it. What do you think?

>> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:73
>>          token.m_location.endOffset = closeBraceOffset + 1;
> 
> Is this `+1` OK for non `CLOSEBRACE` token? For example, If the arrow function expression is splitted by previous line terminators, I think (almost) arbitrary tokens can come here. Correct?

Thanks for hint, I missed that case. I've added new property in parameters to store if line terminator was before token

-- 
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/20150612/02680d1e/attachment-0001.html>