[webkit-reviews] review granted: [Bug 194435] [ESNext] Implement private accessors : [Attachment 420127] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Caio Lima
<ticaiolima at gmail.com>'s request for review:
Bug 194435: [ESNext] Implement private accessors
https://bugs.webkit.org/show_bug.cgi?id=194435

Attachment 420127: Patch

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




--- Comment #15 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 420127
  --> https://bugs.webkit.org/attachment.cgi?id=420127
Patch

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

r=me with comments.

> Source/JavaScriptCore/ChangeLog:37
> +	   new_object	      dst:loc12, inlineCapacity:2 // this is the object
to store getter and setter pair

Can we have a FIXME about using GetterSetter instead?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3000
> +	       if (!excludedNames.contains(entry.key.get())) {
> +		   result.add(entry.key, entry.value);
>		   excludedNames.add(entry.key.get());
> +	       }

contains & add is duplicate cost. Let's do,

auto addResult = excludedNames.add(entry.key.get());
if (addResult.isNewEntry)
    result.add(entry.key, entry.value);

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:583
> +    typedef std::pair<PropertyNode*, PropertyNode*> GetterSetterPair;
> +    typedef HashMap<UniquedStringImpl*, GetterSetterPair, IdentifierRepHash>
GetterSetterMap;

Use `using` instead for new code.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:588
> +    for (; p; p = p->m_next) {

Let's not use p variable here. This is used for the latter iteration. So, let's
move

PropertyListNode* p = this;
RegisterID* dst = nullptr;

just before // Fast case: this loop just handles regular value properties.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:601
> +	   if (!(p->m_node->type() & (PropertyNode::PrivateGetter |
PropertyNode::PrivateSetter)))
> +	       continue;
> +
> +	   // We group private getters and setters to store them in a object
> +	   GetterSetterPair pair(p->m_node,
static_cast<PropertyNode*>(nullptr));
> +	   GetterSetterMap::AddResult result =
privateAccessorMap.add(p->m_node->name()->impl(), pair);
> +	   auto& resultPair = result.iterator->value;
> +	   // If the map already contains an element with node->name(),
> +	   // we need to store this node in the second part.
> +	   if (!result.isNewEntry)
> +	       resultPair.second = p->m_node;
> +	   continue;
> +    }

Can we collect this information into PropertyListNode when parsing?
private getter / setter is not so frequently seen. We should not iterate every
time.
Like,

if (hasPrivateGettersOrSetters()) {
    GetterSetterMap privateAccessorMap;
    for (; p; p = p->m_next) {
	...
    }
    ...
    for (auto& it : privateAccessorMap) {
	...
    }
}

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:603
> +    // Then we declare private accessos

accessos => accessors.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:609
> +	       dst = pair.first->isInstanceClassProperty() ? prototype :
dstOrConstructor;

Let's use block local variable instead of dst.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:628
> +	       generator.emitDirectPutById(getterSetterObj.get(),
setterOrGetterIdent, value.get());
> +	   }
>  
> +	   if (pair.second) {
> +	       dst = pair.second->isInstanceClassProperty() ? prototype :
dstOrConstructor;
> +	       RefPtr<RegisterID> value =
generator.emitNode(pair.second->m_assign);
> +	       if (pair.second->needsSuperBinding())
> +		   emitPutHomeObject(generator, value.get(), dst);
> +	       auto setterOrGetterIdent = pair.second->m_type &
PropertyNode::PrivateGetter
> +		   ? generator.propertyNames().builtinNames().getPrivateName()
> +		   : generator.propertyNames().builtinNames().setPrivateName();
> +	       generator.emitDirectPutById(getterSetterObj.get(),
setterOrGetterIdent, value.get());
> +	   }

They are largely duplicate. Please add lambda to dedupe code.

if (pair.first)
    xxxxx(pair.first);
if (pair.second)
    xxxxx(pair.second);

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:983
>	       RegisterID* privateBrandSymbol =
generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());

Use RefPtr<RegisterID>.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:993
> +	       RegisterID* privateBrandSymbol =
generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());

Use `RefPtr<RegisterID> privateBrandSymbol`.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1008
> +	       RegisterID* privateBrandSymbol =
generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1010
> +	       generator.emitThrowTypeError("Trying to access a not defined
private getter");

"Trying to access an undefined private getter"

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1048
> +	       RegisterID* privateBrandSymbol =
generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1068
>	       generator.emitThrowTypeError("Trying to access a not defined
private setter");

"Trying to access an undefined private setter"

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2413
> +	       RegisterID* privateBrandSymbol =
generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());

Ditto. Please check the same things too.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2424
> +	   RegisterID* privateBrandSymbol =
generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2435
> +	       generator.emitThrowTypeError("Trying to access a not defined
private getter");

"Trying to access an undefined private getter"

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2453
> +	   generator.emitThrowTypeError("Trying to access a not defined private
getter");

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2702
> +	       RegisterID* privateBrandSymbol =
generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2706
> +	       generator.emitThrowTypeError("Trying to access a not defined
private setter");

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2713
> +	   RegisterID* privateBrandSymbol =
generator.emitGetPrivateBrand(generator.newTemporary(), scope.get());

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2723
> +	       generator.emitThrowTypeError("Trying to access a not defined
private getter");

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2741
> +	   generator.emitThrowTypeError("Trying to access a not defined private
getter");

Ditto.

> Source/JavaScriptCore/parser/VariableEnvironment.cpp:129
> +	   if (currentEntry.isSetter() || currentEntry.isMethod() ||
!currentEntry.isGetter())

Is `currentEntry.isSetter() || currentEntry.isMethod()` necessary?

> Source/JavaScriptCore/parser/VariableEnvironment.cpp:173
> +bool VariableEnvironment::declarePrivateGetter(const
RefPtr<UniquedStringImpl>& identifier)
> +{
> +    if (!m_rareData)
> +	   m_rareData = WTF::makeUnique<VariableEnvironment::RareData>();
> +
> +    auto findResult = m_rareData->m_privateNames.find(identifier);
> +
> +    if (findResult == m_rareData->m_privateNames.end()) {
> +	   PrivateNameEntry meta(PrivateNameEntry::Traits::IsDeclared |
PrivateNameEntry::Traits::IsGetter);
> +
> +	   auto entry = VariableEnvironmentEntry();
> +	   entry.setIsPrivateGetter();
> +	   entry.setIsConst();
> +	   entry.setIsCaptured();
> +	   m_map.add(identifier, entry);
> +
> +	   auto addResult = m_rareData->m_privateNames.add(identifier, meta);
> +	   return addResult.isNewEntry;
> +    }

Looks like declarePrivateGetter is largely duplicate to declarePrivateSetter.
Could you dedupe the code?

> Source/JavaScriptCore/parser/VariableEnvironment.cpp:177
> +	   if (currentEntry.isGetter() || currentEntry.isMethod() ||
!currentEntry.isSetter())

Is `currentEntry.isGetter() || currentEntry.isMethod()` necessary?