[Webkit-unassigned] [Bug 146934] [ES6] Arrow function syntax. Arrow function should support the destructuring parameters.

[bugzilla-daemon at webkit.org]

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

Saam Barati <sbarati at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #268951|review?, commit-queue?      |review+, commit-queue-
              Flags|                            |

--- Comment #9 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 268951
  --> https://bugs.webkit.org/attachment.cgi?id=268951
Patch

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

> Source/JavaScriptCore/parser/Parser.cpp:377
> +bool Parser<LexerType>::isArrowFunctionParameters()

This looks good to me, just some really small nits on how to write the function:

> Source/JavaScriptCore/parser/Parser.cpp:382
> +        
> +    if (match(EOFTOK) || (!match(OPENPAREN) && !match(IDENT)))
> +        return isArrowFunction;

I would write this like:
```
if (match(EOFTOK))
    return false;
bool isOpenParen = match(OPENPAREN);
bool isIdent = match(IDENT);
if (!isOpenParen && !isIdent)
    return false;
```
Then, below, you can use these booleans instead of performing the match(...) twice.
I say this because I think that LLVM will CSE this, but lets take matters into our own hand.

> Source/JavaScriptCore/parser/Parser.cpp:386
> +    if (match(IDENT)) {

Then this could be: "if (isIdent) { next(); isArrowFunction = match(ARROWFUNCTION) }

> Source/JavaScriptCore/parser/Parser.cpp:388
> +    } else if (consume(OPENPAREN)) {

Then I would change this to a pure else clause.
Then, I would have the first two lines be:
RELEASE_ASSERT(isOpenParen); // your check at the top of the function guarantees it, so lets not pretend it isn't true.
next();

> Source/JavaScriptCore/parser/Parser.cpp:390
> +            isArrowFunction = consume(CLOSEPAREN) && match(ARROWFUNCTION);

I would write this as:
if {
    next();
    isArrowFunction = match(ARROWFUNCTION);
}

> Source/JavaScriptCore/parser/Parser.cpp:393
> +                // We make fake scope, otherwise parseFormalParameters will add variable to current scope that lead to errors

style: indentation.

-- 
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/20160116/abf54679/attachment-0001.html>