[Webkit-unassigned] [Bug 163208] [ES6]. Implement Annex B.3.3 function hoisting rules for eval

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=163208

GSkachkov <gskachkov at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #294533|1                           |0
        is obsolete|                            |

--- Comment #13 from GSkachkov <gskachkov at gmail.com> ---
Comment on attachment 294533
  --> https://bugs.webkit.org/attachment.cgi?id=294533
Patch

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

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2851
>> +        // Fixme: we do not have llint optimization for those op_codes
> 
> Please file a bug for this or implement the optimization.
> Also, should be FIXME not Fixme.

Just a question, do we need optimization there? I'm not familiar with this part.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:754
>> +    }
> 
> Why is this change needed?

It is allow to fix following code:
eval("'use strict'; class C { constructor() { } }; function foo() { return new C; }; foo()");

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2193
>> +RegisterID* BytecodeGenerator::emitResolveOuterScopeType(RegisterID* dst, RegisterID* scope)
> 
> This function name is incorrect.

Renamed

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2196
>> +    m_codeBlock->addPropertyAccessInstruction(instructions().size());
> 
> This looks wrong.

Removed

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2201
>> +    instructions().append(kill(dst));
> 
> Why not just make dst the result w/out all the tempDestination stuff? This seems like it could be wrong.

Hmm, without    dst = tempDestination(dst), I received segment 11.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2209
>> +    m_codeBlock->addPropertyAccessInstruction(instructions().size());
> 
> This looks wrong.

Removed

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2215
>> +    
> 
> remove newline.

Done

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2217
>> +    instructions().append(localScopeDepth());
> 
> 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?

Ohh, not sure if I got how I can do this.

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1100
>> +                        JSScope* scope = jsCast<JSScope*>(child.value());
> 
> Doesn't LexicalEnvironmentType guarantee that it's a JSSymbolTableObject?

Hmm, it is rewritten function: 
bool JSScope::isVarScope()
{
    if (type() != LexicalEnvironmentType)
        return false;
    return jsCast<JSLexicalEnvironment*>(this)->symbolTable()->scopeType() == SymbolTable::ScopeType::VarScope;
}
So I expect that it should work

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1103
>> +                            result = scope->symbolTable()->scopeType() == SymbolTable::ScopeType::VarScope;
> 
> How we normally do this is:
> if (SymbolTable* symbolTable = scope->symbolTable())
>      ....

Fixed

>> Source/JavaScriptCore/dfg/DFGNode.h:983
>> +    int32_t getOpInfo2()
> 
> Please give this a name for your usage of it.

Renamed to skipScope()

>> Source/JavaScriptCore/dfg/DFGOperations.h:193
>> +JSCell* JIT_OPERATION operationResolveClosestVarScope(ExecState*, JSScope*, UniquedStringImpl*, int32_t);
> 
> should be uint32_t

Done

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8777
>> +    callOperation(operationResolveClosestVarScope, resultGPR, scopeGPR, identifierUID(node->identifierNumber()), node->getOpInfo2());
> 
> We usually give names to the opInfo methods so we know what it's for.

Fixed

>> Source/JavaScriptCore/parser/Parser.h:1182
>> +            // We allways try to hoist function to top scope inside of eval
> 
> I don't think this comment adds anything.

Removed

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:826
>> +    CHECK_EXCEPTION();
> 
> An exception could not be thrown by the previous statement.

Removed

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:832
>> +        result = scope->symbolTable()->scopeType() == SymbolTable::ScopeType::VarScope;
> 
> We usually do:
> else if (SymbolTable* symbolTable = scope->symbolTable())

Fixed

>> Source/JavaScriptCore/runtime/JSScope.cpp:218
>> +JSObject* JSScope::resolveClosestVar(ExecState* exec, JSScope* scope, const Identifier& ident, int skipScope)
> 
> should be unsigned instead of int

Fixed

>> Source/JavaScriptCore/runtime/JSScope.cpp:224
>> +    int skipedScopes = 0;
> 
> typo, should be: "skippedScopes"
> also, should be unsigned.

Removed

-- 
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/20161204/01c2fa06/attachment.html>