[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 #34 from GSkachkov <gskachkov at gmail.com> ---
(In reply to comment #33)
> Comment on attachment 248414 [details]
> 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;

Thank you for this deep review. 
I'm finishing fixing comments from Yusuke Suzuki, after that I'll switch to the fixing of your comments.

-- 
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/20150317/b674290c/attachment-0002.html>