<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. Parser of arrow function with execution as common function"
href="https://bugs.webkit.org/show_bug.cgi?id=144955#c58">Comment # 58</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Implement ES6 arrow function syntax. Parser of arrow function with execution as common function"
href="https://bugs.webkit.org/show_bug.cgi?id=144955">bug 144955</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=254445&action=diff" name="attach_254445" title="Patch">attachment 254445</a> <a href="attachment.cgi?id=254445&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=254445&action=review">https://bugs.webkit.org/attachment.cgi?id=254445&action=review</a>
<span class="quote">>> 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.</span >
Done
<span class="quote">>> 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.</span >
Done
<span class="quote">>> 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.</span >
I've renamed it to parseArrowFunctionExpression.
<span class="quote">>> 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`?</span >
Yep, I've refactored, please take a look if it looks better.
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:1585
>> + position.offset++;
>
> Is it handled when <CR><LF> comes? Hm, this part looks a little bit weird.</span >
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
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:1621
>> + failIfFalse(isEndOfArrowFunction(), "Expected the closing ';' ',' ']' ')' '}' after arrow function");
>
> it seems refering to EOF and Line terminator is needed.</span >
Done
<span class="quote">>> Source/JavaScriptCore/parser/Parser.h:632
>> +
>
> Is the above match(EOFTOK) check needed?</span >
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
<span class="quote">>> Source/JavaScriptCore/parser/SourceCode.h:130
>> + UChar symbol = provider()->source()[endArrowFunction - 1];
>
> I suggest adding `ASSERT(endArrowFunction >= 1);` if it's guaranteed.</span >
Done
<span class="quote">>> 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.)</span >
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?
<span class="quote">>> 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?</span >
Thanks for hint, I missed that case. I've added new property in parameters to store if line terminator was before token</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>