[Webkit-unassigned] [Bug 149338] [ES6] Arrow function created before super() causes TDZ, should it?

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Oct 25 16:19:56 PDT 2015


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

Saam Barati <sbarati at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #264022|review?                     |review-
              Flags|                            |

--- Comment #44 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 264022
  --> https://bugs.webkit.org/attachment.cgi?id=264022
Patch

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

this is looking better and better. Some comments:

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:163
> +    unsigned m_isDerivedConstructorContext: 1;

style: "m_isDerivedConstructorContext:" => "m_isDerivedConstructorContext :"

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:807
> +void BytecodeGenerator::initializeEmptyVarLexicalEnvironmentIfNeeded(SymbolTable* systemTable)

"systemTable" => "symbolTable"
I also don't like the name "initializeEmptyVarLexicalEnvironmentIfNeeded". It doesn't really mean what you're doing here.
I think a better name would be something along the lines of: "initializeArrowFunctionContextScopeIfNeeded".

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:819
> +            systemTable->set(propertyNames().thisIdentifier.impl(), SymbolTableEntry(VarOffset(offset)));

It's worth asserting here that "this" identifier is not in the symbol table already.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:821
> +            if (m_codeType == FunctionCode) {

Aren't there more specific situations where we need this? Like if we're in a derived constructor context?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:849
> +    m_arrowFunctionVarLexicalEnvRegister = emitMove(newRegister(), m_scopeRegister);

I think you want "newBlockScopeVariable" instead of "newRegister"

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2981
> +void BytecodeGenerator::emitUpdateScopeIfNeeded()

I think a better name for this would be something like:
"emitUpdateArrowFunctionContextScope".
This names seems to general. It would be good to pin it down to being arrowfunction-related.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2996
> +void BytecodeGenerator::emitPutThisToScope()

Again, I would signify in the name that this is related to arrow functions.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3897
> +void BytecodeGenerator::emitGetNewTargeFromArrowFunctionLexicalEnvironment()

typo: "emitGetNewTarge..." => "emitGetNewTarget..."

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:827
> +        RegisterID* m_arrowFunctionVarLexicalEnvRegister { nullptr };

nit: I would call this something like: "m_arrowFunctionContextLexicalEnvironmentRegister".
I think "var" is the wrong word here.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:828
> +        RegisterID* m_arrowFunctionResolvedLexicalEnvRegister { nullptr };

style: I would spell out "Environment" in this name, even though it's verbose.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:699
> +// We need try to load 'this' before call eval, because it can created by 'super' in some of the arrow function

style: Indent this.
Also, I'm a bit confused about this. Do we get the current function's "this" when making a function call?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:738
> +                && (generator.usesArrowFunction() || generator.usesEval())))

style: I'd move this to the above line.
WebKit style says that this should be 4 spaces from the indentation of the "if", which would look bad here. I would just move it one line up.

-- 
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/20151025/cbf210ed/attachment-0001.html>


More information about the webkit-unassigned mailing list