<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:utatane.tea@gmail.com" title="Yusuke Suzuki <utatane.tea@gmail.com>"> <span class="fn">Yusuke Suzuki</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Implement ES6 arrow function syntax. Parser of arrow function with execution as common function"
href="https://bugs.webkit.org/show_bug.cgi?id=144955">bug 144955</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #255191 Flags</td>
<td>review?, commit-queue?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Implement ES6 arrow function syntax. Parser of arrow function with execution as common function"
href="https://bugs.webkit.org/show_bug.cgi?id=144955#c67">Comment # 67</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Implement ES6 arrow function syntax. Parser of arrow function with execution as common function"
href="https://bugs.webkit.org/show_bug.cgi?id=144955">bug 144955</a>
from <span class="vcard"><a class="email" href="mailto:utatane.tea@gmail.com" title="Yusuke Suzuki <utatane.tea@gmail.com>"> <span class="fn">Yusuke Suzuki</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=255191&action=diff" name="attach_255191" title="Patch">attachment 255191</a> <a href="attachment.cgi?id=255191&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=255191&action=review">https://bugs.webkit.org/attachment.cgi?id=255191&action=review</a>
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.
<span class="quote">> Source/JavaScriptCore/ChangeLog:60
> + (JSC::SourceProviderCacheItem::endArrowFunctionToken):</span >
The previous one remains here. Let's drop it :)
<span class="quote">> Source/JavaScriptCore/ChangeLog:106
> + (JSC::SyntaxChecker::setFunctionNameStart):</span >
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.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1270
> + result = parseArrowFunctionExpression(context, parseType);</span >
Saam's pointing is reasonable. Could you rename it to "parseArrowFunctionSingleExpressionBody"?
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1447
> + bool isClassConstructor;</span >
Let's drop this line.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1472
> + isClassConstructor = mode == MethodMode && info.name && *info.name == m_vm->propertyNames->constructor;</span >
It seems this line is not necessary, right?
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1478
> + isClassConstructor = constructorKind != ConstructorKind::None;</span >
This always override the previous one. And I think this line is not necessary.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1487
> + isClassConstructor = false;</span >
I think this line is not necessary. Instead, let's insert `ASSERT(constructorKind == ConstructorKind::None);` (correct?)
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:-1387
> - bool isClassConstructor = constructorKind != ConstructorKind::None;</span >
I think using this is enough because constructorKind for `ArrowFunctionParseType` is always ConstructorKind::None.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1495
> + constructorKind = isClassConstructor ? constructorKind : ConstructorKind::None;</span >
I think this line is not necessary.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1610
> + info.isEndByTerminator = true;</span >
OK, they are initialized in struct definition with C++11 initialization form.
<span class="quote">> Source/JavaScriptCore/parser/Parser.cpp:1635
> + parameters.isPrevTerminator = m_lexer->prevTerminator();</span >
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.
<span class="quote">> Source/JavaScriptCore/parser/ParserFunctionInfo.h:41
> + unsigned startFunctionOffset = 0;</span >
Let's exchange this member's order.
unsigned startFunctionOffset = 0;
unsigned endFunctionOffset = 0;
<span class="quote">> Source/JavaScriptCore/parser/ParserFunctionInfo.h:49
> + FunctionBodyType functionBodyType = StandardFunctionBodyBlock;</span >
They are initialized with C++ initialization form.
<span class="quote">> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:39
> + unsigned endFunctionOffset;</span >
Since now we have endFunctionEndOffset, endFunctionStartOffset sounds better.
<span class="quote">> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:50
> + bool isPrevTerminator;</span >
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 };</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>