[webkit-reviews] review granted: [Bug 174212] [JSC] Add support for instance class fields : [Attachment 387361] Part 1: Public Instance Fields (rev4)

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Caitlin Potter (:caitp)
<caitp at igalia.com>'s request for review:
Bug 174212: [JSC] Add support for instance class fields
https://bugs.webkit.org/show_bug.cgi?id=174212

Attachment 387361: Part 1: Public Instance Fields (rev4)

https://bugs.webkit.org/attachment.cgi?id=387361&action=review




--- Comment #184 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 387361
  --> https://bugs.webkit.org/attachment.cgi?id=387361
Part 1: Public Instance Fields (rev4)

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

r=me with comments. Nice.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:86
> +    if (info.needsClassFieldInitializer() ==
NeedsClassFieldInitializer::Yes) {
> +	   createRareDataIfNecessary();
> +	   m_rareData->m_needsClassFieldInitializer =
static_cast<unsigned>(NeedsClassFieldInitializer::Yes);
> +    }

Nice

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:217
> +    Vector<JSTextPosition>* instanceFieldLocations() const

Yeah, elegant code is nice. But I love performance benefit, so if I need to
take one of either, I always take performance benefit :P

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:228
> +	   ensureRareData().m_instanceFieldLocations = instanceFieldLocations;

Let's do `WTFMove(instanceFieldLocations)` to avoid copying.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1134
> +		   if (constructorKind() == ConstructorKind::Extends ||
isDerivedConstructorContext()) {
>		       newDerivedContextType =
DerivedContextType::DerivedConstructorContext;
> +		       needsClassFieldInitializer =
m_codeBlock->needsClassFieldInitializer();
> +		   }

This code means that, FunctionMetadataNode::needsClassFieldInitializer could be
wrong (Even if it is saying NO, it depends on the upper CodeBlock's value).
So, I don't think FunctionMetadataNode::needsClassFieldInitializer's name is
correct.
Please rename FunctionMetadataNode::needsClassFieldInitializer to appropriate
one, and ensure it is appropriately set. 
For example,

FunctionMetadataNode::isConstructorAndNeedsClassFieldInitializer
And
UnlinkedCodeBlock::needsClassFieldInitializer

> Source/JavaScriptCore/parser/Nodes.cpp:333
> +bool PropertyListNode::shouldCreateLexicalScopeForClass(PropertyListNode*
list)

Add FIXME+url to avoid this behavior. While collecting property lists, I think
we can calculate it.

> Source/JavaScriptCore/parser/Nodes.cpp:345
> +bool PropertyListNode::hasInstanceFields() const

Ditto.

> Source/JavaScriptCore/parser/Parser.cpp:2964
> +	   } else if (Options::useClassFields() && !match(OPENPAREN) && tag ==
ClassElementTag::Instance && parseMode == SourceParseMode::MethodMode &&
!isGetter && !isSetter) {

`!isGetter && !isSetter` is not necessary since we have `if (isGetter ||
isSetter) {`

Put ASSERT(!isGetter && !isSetter) inside brace.