[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

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

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

Patch looks good to me. Great work.
Some suggestions below.

> Source/JavaScriptCore/ChangeLog:9
> +        In patch were implemented the simplest cases of arrow function declaration:

"In patch were implemented" => "This patch implements"

> Source/JavaScriptCore/ChangeLog:17
> +          2 raising exception in case of trying to use ‘new’ with arrow function

Nit: Looks like some weird characters around the "new" here.

> Source/JavaScriptCore/parser/ASTBuilder.h:372
> +            : m_sourceCode->subExpression(info.openBraceOffset, info.closeBraceOffset, info.bodyStartLine, info.bodyStartColumn);

Nit: It may be worth renaming ParserFunctionInfo's "openBraceOffset" variable to "bodyStartOffset" or something like that and then this ternary expression becomes unnecessary.

> Source/JavaScriptCore/parser/Parser.cpp:1968
> +template <class TreeBuilder> TreeExpression Parser<LexerType>::parseArrowFunctionExpression(TreeBuilder& context)

In regards to the controlFlowProfiler, this code is ok. To be sure and prevent regressions, it's worth writing a few simple tests for the controlFlowProfiler.
If you look inside JavaScriptCore/tests/controlFlowProfiler/* there is a standardized way of doing this. You can do this in another patch or you can
open a bug and assign it to me and I'll write some tests for it. 

On another note, I don't think this code path is worthy of being its own function. I'd just inline this inside parseArrowFunctionStatement because
this looks like it's a simple wrapper around parseAssignmentExpression.

> Source/JavaScriptCore/parser/Parser.cpp:2026
> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)

Nit: Would this whole sequence of checking if something is an arrow function be worthy of its own function?

> Source/JavaScriptCore/parser/Parser.cpp:2034
> +            if (match(ARROWFUNCTION))

This code seems unnecessarily convoluted. I'd move the below code inside the "if (match(ARROWFUNCTION))" into this branch.
If we reach this branch we're guaranteed to execute the code of parseArrowFunctionWrapper in the below branch.
That way we don't have to worry about the arrowFunctionEmptyList variable and we don't duplicate code inside the "#if/#else".

> Source/JavaScriptCore/parser/SourceCode.h:129
> +        ASSERT(provider()->source()[endArrowFunction - 1] == ',' || provider()->source()[endArrowFunction - 1] == ';' || provider()->source()[endArrowFunction - 1] == ')' || provider()->source()[endArrowFunction - 1] == ']' || provider()->source()[endArrowFunction - 1] == '}');

I'd just cache the character in a local variable and remove the Fixme and switch to ASSERT_UNUSED.

> Source/JavaScriptCore/tests/stress/arrowfunction-asparamter-1.js:1
> +// Inspired by mozilla tests

I think some of these tests don't make sense as "stress tests". 
Maybe they're better inside LayoutTests where  we have some similar tests.

-- 
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/20150526/47aaec75/attachment-0001.html>