[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