<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [JSC] implement async functions proposal"
href="https://bugs.webkit.org/show_bug.cgi?id=156147#c74">Comment # 74</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [JSC] implement async functions proposal"
href="https://bugs.webkit.org/show_bug.cgi?id=156147">bug 156147</a>
from <span class="vcard"><a class="email" href="mailto:caitp@igalia.com" title="Caitlin Potter (:caitp) <caitp@igalia.com>"> <span class="fn">Caitlin Potter (:caitp)</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=277938&action=diff" name="attach_277938" title="Patch">attachment 277938</a> <a href="attachment.cgi?id=277938&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=277938&action=review">https://bugs.webkit.org/attachment.cgi?id=277938&action=review</a>
I'll have these addressed shortly, thanks for the look
<span class="quote">>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:630
>> + if (SourceParseMode::ArrowFunctionMode == parseMode || SourceParseMode::AsyncArrowFunctionBodyMode == parseMode || SourceParseMode::AsyncArrowFunctionMode == parseMode || SourceParseMode::AsyncArrowFunctionMode == parseMode) {
>
> AsyncArrowFunctionMode is duplicated.</span >
Oops, good catch
<span class="quote">>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2906
>> + ASSERT(func->metadata()->parseMode() == SourceParseMode::ArrowFunctionMode || func->metadata()->parseMode() == SourceParseMode::AsyncArrowFunctionMode);
>
> Is it safe for AsyncArrowFunctionBodyMode with single line expression?</span >
AsyncArrowFunctionBodyMode never gets here, since it's just a normal function
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:255
>> + bool isArrowFunctionBodyExpression = parseMode == SourceParseMode::AsyncArrowFunctionBodyMode && !match(OPENBRACE);
>
> Can we move this `parseMode == SourceParseMode::AsyncArrowFunctionBodyMode` to L263?</span >
Unfortunately, this matters whether the function is being re-parsed or not --- this breaks if this isn't known here.
There might be a better place to move this to, but it can't be in the `isReparsingFunction()` conditional.
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:619
>> + // Eagerly parse as AsyncFUnctionDeclaration. This is the uncommon case,
>
> Typo: AsyncFUnctionDeclaration => AsyncFunctionDeclaration</span >
Right, thanks
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:625
>> + result = parseAsyncFunctionDeclaration(context);
>
> Let's break here, call restoreSavePoint(savePoint); and fall through to the await / field path.</span >
Alright
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:1796
>> + result = parseAsyncFunctionDeclaration(context);
>
> These code is largely the same to the FUNCTION case. So extracting this to some function is recommended.</span >
Sorry, can you clarify what you mean here?
I'm not sure if you're referring to this entire case statement, or the inside of `parseAsyncFunctionDeclaration` (which could share some code with parseFunctionDeclaration maybe), or what
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:1797
>> + }
>
> Nit: breaking here may clean up the following code :)</span >
Okay
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:2268
>> + }
>
> These checks are now done in parsing location. Let's remove this.
> When removing this, maybe, we need to recheck that closestParentOrdinaryFunctionNonLexicalScope works correctly with async arrow wrapper / body functions.</span >
Yeah, this stuff is gone in the rebase so far --- and seems to do the right thing still
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:3197
>> + next();
>
> Is it necessary to create a save point here? Can we just call `semanticFailIfFalse(match(FUNCTION) && !m_lexer->prevTerminator(), "...")` ?</span >
Re-reading the spec, it looks like you're right --- I was thinking ASI would matter here, but I suppose not.
Good catch!
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:4014
>> + }
>
> Which tests need this?</span >
Ah --- previously, I had rewritten the arrow function parsing stuff to pay attention to the error classifier, which potentially saves it a bit of time. However at this point, this is dead code, I thought I had removed this already. The goal was to help reparse arrow functions less frequently, but at this point it's unnecessary
<span class="quote">>> Source/JavaScriptCore/parser/Parser.cpp:4221
>> + failIfFalse(base, "Failed to parse async function expression");
>
> Why is this handled here? Can we handle `async function` case in parsePrimaryExpression?
> And can we parse async arrow function in parseAssignmentExpression? (At that case, we check `async IDENT =>` in parsePrimaryExpression similar to the current arrow function parsing).
>
> <a href="https://tc39.github.io/ecmascript-asyncawait/#prod-UnaryExpression">https://tc39.github.io/ecmascript-asyncawait/#prod-UnaryExpression</a></span >
We do parse async arrow functions in `parseAssignmentExpression`.
I can probably move the AsyncFunctionExpression parsing to `parsePrimaryExpression`, though.
<span class="quote">>> Source/JavaScriptCore/runtime/CommonIdentifiers.h:36
>> + macro(AsyncFunction) \
>
> Let's remove this.</span >
Okay</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>