<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:sbarati@apple.com" title="Saam Barati <sbarati@apple.com>"> <span class="fn">Saam Barati</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6]. Implement Annex B.3.3 function hoisting rules for eval"
href="https://bugs.webkit.org/show_bug.cgi?id=163208">bug 163208</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 #294533 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6]. Implement Annex B.3.3 function hoisting rules for eval"
href="https://bugs.webkit.org/show_bug.cgi?id=163208#c9">Comment # 9</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6]. Implement Annex B.3.3 function hoisting rules for eval"
href="https://bugs.webkit.org/show_bug.cgi?id=163208">bug 163208</a>
from <span class="vcard"><a class="email" href="mailto:sbarati@apple.com" title="Saam Barati <sbarati@apple.com>"> <span class="fn">Saam Barati</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=294533&action=diff" name="attach_294533" title="Patch">attachment 294533</a> <a href="attachment.cgi?id=294533&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=294533&action=review">https://bugs.webkit.org/attachment.cgi?id=294533&action=review</a>
I think the main approach is OK, but there are a lot of little things I think might be wrong or could be better/simpler. Comments below.
<span class="quote">> Source/JavaScriptCore/ChangeLog:13
> + Current patch implement Annex B.3.3 that is related to hoisting of function
> + declaration in eval. <a href="https://tc39.github.io/ecma262/#sec-web-compat-evaldeclarationinstantiation">https://tc39.github.io/ecma262/#sec-web-compat-evaldeclarationinstantiation</a>
> + Function declaration in eval should create variable with function name in function scope
> + where eval is invoked or bind to variable if it declared outside of the eval. If variable is created
> + it can be removed by delete a; command. If eval is invoke in block scope that
> + contains let/const variable with the same name as function declaration we do not binding</span >
It's worth also talking about your implementation in the changelog.
<span class="quote">> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2851
> + // Fixme: we do not have llint optimization for those op_codes</span >
Please file a bug for this or implement the optimization.
Also, should be FIXME not Fixme.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:754
> + auto varKind = [&] (UniquedStringImpl* uid) -> VarKind {
> + return evalNode->captures(uid) ? VarKind::Scope : VarKind::Stack;
> + };
> +
> + for (FunctionMetadataNode* function : evalNode->functionStack()) {
> + VarKind kind = varKind(function->ident().impl());
> + if (kind == VarKind::Scope)
> + m_codeBlock->addFunctionDecl(makeFunction(function));
> + else
> + m_functionsToInitialize.append(std::make_pair(function, GlobalFunctionVariable));
> + }</span >
Why is this change needed?
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2193
> +RegisterID* BytecodeGenerator::emitResolveOuterScopeType(RegisterID* dst, RegisterID* scope)</span >
This function name is incorrect.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2196
> + m_codeBlock->addPropertyAccessInstruction(instructions().size());</span >
This looks wrong.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2201
> + dst = tempDestination(dst);
> + emitOpcode(op_is_scope_var_type);
> + instructions().append(kill(dst));</span >
Why not just make dst the result w/out all the tempDestination stuff? This seems like it could be wrong.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2209
> + m_codeBlock->addPropertyAccessInstruction(instructions().size());</span >
This looks wrong.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2215
> + </span >
remove newline.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2217
> + instructions().append(localScopeDepth());</span >
Why not just make the access off the base-most scope of the eval instead of having a local scope depth? We should already have the scope on the symbol table stack, right?
<span class="quote">> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1100
> + JSScope* scope = jsCast<JSScope*>(child.value());</span >
Doesn't LexicalEnvironmentType guarantee that it's a JSSymbolTableObject?
<span class="quote">> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1103
> + if (scope->symbolTable() != nullptr)
> + result = scope->symbolTable()->scopeType() == SymbolTable::ScopeType::VarScope;</span >
How we normally do this is:
if (SymbolTable* symbolTable = scope->symbolTable())
....
<span class="quote">> Source/JavaScriptCore/dfg/DFGNode.h:983
> + int32_t getOpInfo2()</span >
Please give this a name for your usage of it.
<span class="quote">> Source/JavaScriptCore/dfg/DFGOperations.h:193
> +JSCell* JIT_OPERATION operationResolveClosestVarScope(ExecState*, JSScope*, UniquedStringImpl*, int32_t);</span >
should be uint32_t
<span class="quote">> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8777
> + callOperation(operationResolveClosestVarScope, resultGPR, scopeGPR, identifierUID(node->identifierNumber()), node->getOpInfo2());</span >
We usually give names to the opInfo methods so we know what it's for.
<span class="quote">> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:821
> + linkSlowCase(iter);
> + JITSlowPathCall slowPathCall(this, currentInstruction, slow_path_is_scope_var_type);
> + slowPathCall.call();</span >
This should not have a slow path. The fast path should do the function call.
<span class="quote">> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:828
> + linkSlowCase(iter);
> + JITSlowPathCall slowPathCall(this, currentInstruction, slow_path_resolve_closest_var_scope);
> + slowPathCall.call();</span >
ditto
<span class="quote">> Source/JavaScriptCore/parser/Parser.h:1182
> + // We allways try to hoist function to top scope inside of eval</span >
I don't think this comment adds anything.
<span class="quote">> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:826
> + CHECK_EXCEPTION();</span >
An exception could not be thrown by the previous statement.
<span class="quote">> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:832
> + else if (scope->symbolTable() != nullptr)
> + result = scope->symbolTable()->scopeType() == SymbolTable::ScopeType::VarScope;</span >
We usually do:
else if (SymbolTable* symbolTable = scope->symbolTable())
<span class="quote">> Source/JavaScriptCore/runtime/JSScope.cpp:218
> +JSObject* JSScope::resolveClosestVar(ExecState* exec, JSScope* scope, const Identifier& ident, int skipScope)</span >
should be unsigned instead of int
<span class="quote">> Source/JavaScriptCore/runtime/JSScope.cpp:224
> + int skipedScopes = 0;</span >
typo, should be: "skippedScopes"
also, should be unsigned.
<span class="quote">> Source/JavaScriptCore/runtime/JSScope.cpp:245
> + if (skipedScopes < skipScope) {
> + ++skipedScopes;
> + continue;
> + }</span >
Why not just have the bytecode emit starting from the base-most scope of the function and you can remove the skipScope parameter entirely?</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>