<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:gskachkov@gmail.com" title="GSkachkov <gskachkov@gmail.com>"> <span class="fn">GSkachkov</span></a>
</span> changed
<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>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #255506 is obsolete</td>
<td>1
</td>
<td>
</td>
</tr></table>
<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#c84">Comment # 84</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=""><a href="attachment.cgi?id=255506&action=diff" name="attach_255506" title="Patch">attachment 255506</a> <a href="attachment.cgi?id=255506&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=255506&action=review">https://bugs.webkit.org/attachment.cgi?id=255506&action=review</a>
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:365
>> + if ((functionParseType == ArrowFunctionParseType && isEndOfArrowFunction()) || !arrowfunctionStatement) {
>
> Why this branch is needed?
> Is it possible to write just the following here?
>
> propagateError();
> return sourceElements;
>
> Since this branch is only used for body = expr case. So if we parse one assignment expression in parseArrowFunctionSingleExpressionBody, no further parsing should not be allowed.</span >
Good shoot, before it was necessary. Done.
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:368
>> + }
>
> Is here's fall through correct?</span >
I've removed propagateError() statement
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:610
>> + failDueToUnexpectedToken();
>
> OK, please insert comment about reparsing phase and parseType relation like the following.
>
> // When reparsing phase, parseType becomes StandardFunctionParseType even if the function is arrow function.
> // This condition considers the following situations.
> // (1): If we are in the reparsing phase, this arrow function is already parsed once, so there is no syntax error.
> // (2): But if we are not in the reparsing phase, we should check this function is called in the context of the arrow function.</span >
Done
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:631
>> + if (parseType == StandardFunctionParseType)
>
> Change it to `m_lexer->isReparsing()`. `parseType == StandardFunctionParseType` is not essential.
>
> BTW, this code path is a little bit complicated. I recommend to insert comment here.
>
> // Only when reparsing phase, our parsing target contains the EndOfArrowFunction token at the end of the code.
> // So consuming the last token here is needed.
>
> Correct?</span >
I'm not sure. It is possible when state of lexer is reparsing because of reparsing of wrapped function, but not arrow function. For instance test with (function funcSelfExecAE1(value) { var f = x => x+1; return f(value);})(123); fails because parser is reparsing funcSelfExecAE1 function.
Anyway I get rid of this condition, after refactoring of the endFunctionOffset calculation
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:1567
>> + }
>
> Drop them.</span >
Not sure that I need to do this.
I've made small refactoring those line possible it will be more clear what is going on in this lines.
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:1590
>> + info.startFunctionOffset = m_token.m_data.offset;
>
> This line is not needed since it's already set.
> I think info.startFunctionOffset should be set before looking up the cache and should not be touched after that.</span >
The line above is moving to next token, so we need reset info.startFunctionOffset again.
I've made small refactoring of this conditions to avoid nested conditions, hope it will be more readable.
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:1625
>> + info.endFunctionOffset = location.endOffset + 1;
>
> Modifying offset is dangerous I think. It easily leads heap overflow.
> And as described below, it leads `parameters.endFunctionStartOffset > parameters.endFunctionEndOffset`.
>
> I suggest always storing locationBeforeLastToken() in bodyexpression case. And drop prevTerminator information.
> It ensures the following invariant.
>
> 1. the stored token is always the last token in the function's body. if it using braces ({ ... }), closed brace is stored. And if it takes expr style ((x) => x), the last token `x` is stored.
>
> And when recovering from it, always consume the token.</span >
OK. Agree, done
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:1629
>> + info.isEndByTerminator = m_lexer->prevTerminator();
>
> Drop isEndByTerminator information.</span >
Done
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:1647
>> + parameters.endFunctionEndOffset = endFunctionEndOffset;
>
> In `m_lexer->prevTerminator()` case, `info.endFunctionOffset` = `location.endOffset + 1`. And `endFunctionEndOffset` is `location.endOffset`.
> So now, `parameters.endFunctionStartOffset > parameters.endFunctionEndOffset`. Is it indended?</span >
Fixed and used endFunctionEndOffset variables.
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:1663
>> + if ((parseType == ArrowFunctionParseType && info.functionBodyType == ArrowFunctionBodyBlock) || parseType != ArrowFunctionParseType) {
>
> Use `parseType == StandardFunctionParseType` instead of `parseType != ArrowFunctionParseType`.</span >
Done</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>