[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

GSkachkov <gskachkov at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #255506|1                           |0
        is obsolete|                            |

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

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

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

Good shoot, before it was necessary. Done.

>> Source/JavaScriptCore/parser/Parser.cpp:368
>> +        }
> 
> Is here's fall through correct?

I've removed propagateError() statement

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

Done

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

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

>> Source/JavaScriptCore/parser/Parser.cpp:1567
>> +        }
> 
> Drop them.

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.

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

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.

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

OK. Agree, done

>> Source/JavaScriptCore/parser/Parser.cpp:1629
>> +        info.isEndByTerminator = m_lexer->prevTerminator();
> 
> Drop isEndByTerminator information.

Done

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

Fixed and used endFunctionEndOffset variables.

>> Source/JavaScriptCore/parser/Parser.cpp:1663
>> +    if ((parseType == ArrowFunctionParseType && info.functionBodyType == ArrowFunctionBodyBlock) || parseType != ArrowFunctionParseType) {
> 
> Use `parseType == StandardFunctionParseType` instead of `parseType != ArrowFunctionParseType`.

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/20150625/65cebede/attachment-0001.html>