[Webkit-unassigned] [Bug 140855] [ES6] Implement ES6 arrow function syntax

[bugzilla-daemon at webkit.org]

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

--- Comment #33 from Saam Barati <saambarati1 at gmail.com> ---
Comment on attachment 248414
  --> https://bugs.webkit.org/attachment.cgi?id=248414
Patch

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

Patch looks good overall. I think you can touch it up a bit before landing.

> Source/JavaScriptCore/ChangeLog:16
> +

It's worth going through some of this to check for spelling/grammatical errors. There are many 
of these such errors.

Also, it's nice to have some annotations of the functions you changed below,
but stay away from restating what the changelog already states. Your comments
should give insight as to why you changed something or how something works.
It's not worth saying something like: "Added extra parameter". The diff below
will tell us this.

> Source/JavaScriptCore/parser/ASTBuilder.h:-314
> -    

You should revert this whitespace and all other white space changes below.

> Source/JavaScriptCore/parser/Parser.cpp:584
> +    semanticFailIfFalse(currentScope()->isFunction(), "Return statements are only valid inside arrow functions");

I'm confused by this error message here. Is it actually supposed to say that?

> Source/JavaScriptCore/parser/Parser.cpp:1502
> +template <class TreeBuilder> bool Parser<LexerType>::parseArrowFunctionInfo(TreeBuilder& context, FunctionParseMode mode, ConstructorKind constructorKind, ParserFunctionInfo<TreeBuilder>& info)

This function looks like it has a ton of duplicated code with parseFunctionInfo(.).
It's worth creating an abstraction that handles both function info's so we don't
end up with two divergent code paths in the future.

> Source/JavaScriptCore/parser/Parser.cpp:1524
> +            info.parameters = parseFormalParameters(context);

I don't think this will do what you want. This will parse:
x,y => {}
I think the syntax only allows for single parameter arrow functions to not have parens around
their formal parameter list.
You will probably want to failIfFalse if there isn't only a single parameter here

> Source/JavaScriptCore/parser/Parser.cpp:1642
> +        bool isEndOfArrowFunction = match(SEMICOLON) || match(COMMA) || match(CLOSEPAREN) || match(CLOSEBRACE) || match(CLOSEBRACKET);

Nit: It may be worth writing a function to do this since
you have almost this same expression above.

> Source/JavaScriptCore/parser/Parser.h:84
> +enum FunctionParseType { StandardFunctionParseType, ArrowFunctionParseType };

I think we should use a better name for this because parseSourceElements(.) is used to parse more than functions.
Maybe something as simple as SourceElementsParseType

> Source/JavaScriptCore/tests/stress/arrowfunction.js:1
> +var result;

Is there another bug open for the correct implementation of the arrow function to have
lexical "this" scoping?

I think it's probably okay to land this in two different bugs, but you'll probably end up changing some 
of the code in this patch.

We should also have a lot of parsing tests for this patch on top of these tests.
For example, I think your patch as it stands will incorrectly parse:
x,y => x + y;

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150316/cb244bb3/attachment-0002.html>