[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

Yusuke Suzuki <utatane.tea at gmail.com> changed:

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

--- Comment #67 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 255191
  --> https://bugs.webkit.org/attachment.cgi?id=255191
Patch

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

Great work. Still a few nits remmain. But logic looks good to me :-)
It seems that there are some unitialized fields. Please ensure that all fields are initialized even in StandardFunction type.

> Source/JavaScriptCore/ChangeLog:60
> +        (JSC::SourceProviderCacheItem::endArrowFunctionToken):

The previous one remains here. Let's drop it :)

> Source/JavaScriptCore/ChangeLog:106
> +        (JSC::SyntaxChecker::setFunctionNameStart):

Ah, when using `Tools/Scripts/webkit-patch upload --update-changelogs` and the ChangeLog is significantly different from the previous version, it appends new logs.
So let's drop the previous one manually.

> Source/JavaScriptCore/parser/Parser.cpp:1270
> +        result = parseArrowFunctionExpression(context, parseType);

Saam's pointing is reasonable. Could you rename it to "parseArrowFunctionSingleExpressionBody"?

> Source/JavaScriptCore/parser/Parser.cpp:1447
> +    bool isClassConstructor;

Let's drop this line.

> Source/JavaScriptCore/parser/Parser.cpp:1472
> +        isClassConstructor = mode == MethodMode && info.name && *info.name == m_vm->propertyNames->constructor;

It seems this line is not necessary, right?

> Source/JavaScriptCore/parser/Parser.cpp:1478
> +        isClassConstructor = constructorKind != ConstructorKind::None;

This always override the previous one. And I think this line is not necessary.

> Source/JavaScriptCore/parser/Parser.cpp:1487
> +        isClassConstructor = false;

I think this line is not necessary. Instead, let's insert `ASSERT(constructorKind == ConstructorKind::None);` (correct?)

> Source/JavaScriptCore/parser/Parser.cpp:-1387
> -    bool isClassConstructor = constructorKind != ConstructorKind::None;

I think using this is enough because constructorKind for `ArrowFunctionParseType` is always ConstructorKind::None.

> Source/JavaScriptCore/parser/Parser.cpp:1495
> +    constructorKind = isClassConstructor ? constructorKind : ConstructorKind::None;

I think this line is not necessary.

> Source/JavaScriptCore/parser/Parser.cpp:1610
> +        info.isEndByTerminator = true;

OK, they are initialized in struct definition with C++11 initialization form.

> Source/JavaScriptCore/parser/Parser.cpp:1635
> +            parameters.isPrevTerminator = m_lexer->prevTerminator();

These fields are not initialized if parseType is not ArrowFunctionParseType.
Since they are primimtive types (unsigned int etc.), use of uninitialized values causes undefined behavior in C++.

Let's take the either way
1. initialize them in the struct definition with C++11 initialization form.
2. initialize them before this if-branch.

> Source/JavaScriptCore/parser/ParserFunctionInfo.h:41
> +    unsigned startFunctionOffset = 0;

Let's exchange this member's order.
    unsigned startFunctionOffset = 0;
    unsigned endFunctionOffset = 0;

> Source/JavaScriptCore/parser/ParserFunctionInfo.h:49
> +    FunctionBodyType functionBodyType = StandardFunctionBodyBlock;

They are initialized with C++ initialization form.

> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:39
> +    unsigned endFunctionOffset;

Since now we have endFunctionEndOffset, endFunctionStartOffset sounds better.

> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:50
> +    bool isPrevTerminator;

Since the above 3 fields not set if the function is standard function, it becomes undefined values in C++.
Let's use C++11 initialization.

    bool isBodyArrowExpression { false };
    JSTokenType tokenType { CLOSEBRACE };
    bool isPrevTerminator { false };

-- 
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/20150619/50f657fb/attachment-0001.html>