Private accessors are specified on https://tc39.github.io/proposal-private-methods/.
Created attachment 370695 [details] WIP - Patch This Patch is dependent on https://bugs.webkit.org/show_bug.cgi?id=194434. WIP
Created attachment 373127 [details] WIP - Patch This Patch doesn't apply to trunk, since it is based on top of https://bugs.webkit.org/show_bug.cgi?id=194434
Created attachment 391861 [details] WIP - Patch
Created attachment 391866 [details] WIP - Patch
*** Bug 219182 has been marked as a duplicate of this bug. ***
Created attachment 419845 [details] Patch
Created attachment 419866 [details] Patch
Comment on attachment 419866 [details] Patch I've found a couple of bugs when using private identifiers instead of common identifiers.
Created attachment 419911 [details] Patch
Created attachment 419994 [details] Patch
Comment on attachment 419994 [details] Patch r- because I've found another bug on postfix/prefix nodes.
Created attachment 420052 [details] WIP Patch
Comment on attachment 420052 [details] WIP Patch This is adding initial support for private accessors and postfix/prefix nodes. Tests are still needed.
Created attachment 420127 [details] Patch
Comment on attachment 420127 [details] 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?
Comment on attachment 420127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420127&action=review Thank you very much for the review! >> 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? Definitely yes. >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3000 >> + } > > contains & add is duplicate cost. Let's do, > > auto addResult = excludedNames.add(entry.key.get()); > if (addResult.isNewEntry) > result.add(entry.key, entry.value); Done. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:583 >> + typedef HashMap<UniquedStringImpl*, GetterSetterPair, IdentifierRepHash> GetterSetterMap; > > Use `using` instead for new code. Done. >> 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. Done. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:601 >> + } > > 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) { > ... > } > } Done. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:609 >> + dst = pair.first->isInstanceClassProperty() ? prototype : dstOrConstructor; > > Let's use block local variable instead of dst. Done. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:628 >> + } > > They are largely duplicate. Please add lambda to dedupe code. > > if (pair.first) > xxxxx(pair.first); > if (pair.second) > xxxxx(pair.second); Done. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:983 >> RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get()); > > Use RefPtr<RegisterID>. Done. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:993 >> + RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get()); > > Use `RefPtr<RegisterID> privateBrandSymbol`. Done. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1010 >> + generator.emitThrowTypeError("Trying to access a not defined private getter"); > > "Trying to access an undefined private getter" I changed it, but I'm a bit worried that this can cause some confusion about about JS `undefined` value. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1048 >> + RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get()); > > Ditto. Done. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1068 >> generator.emitThrowTypeError("Trying to access a not defined private setter"); > > "Trying to access an undefined private setter" Done. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2413 >> + RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get()); > > Ditto. Please check the same things too. Done. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2424 >> + RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get()); > > Ditto. Done. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2435 >> + generator.emitThrowTypeError("Trying to access a not defined private getter"); > > "Trying to access an undefined private getter" Done. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2453 >> + generator.emitThrowTypeError("Trying to access a not defined private getter"); > > Ditto. Done. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2702 >> + RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get()); > > Ditto. Done. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2706 >> + generator.emitThrowTypeError("Trying to access a not defined private setter"); > > Ditto. Done. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2713 >> + RegisterID* privateBrandSymbol = generator.emitGetPrivateBrand(generator.newTemporary(), scope.get()); > > Ditto. Done. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2723 >> + generator.emitThrowTypeError("Trying to access a not defined private getter"); > > Ditto. Done. >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2741 >> + generator.emitThrowTypeError("Trying to access a not defined private getter"); > > Ditto. Done. >> Source/JavaScriptCore/parser/VariableEnvironment.cpp:129 >> + if (currentEntry.isSetter() || currentEntry.isMethod() || !currentEntry.isGetter()) > > Is `currentEntry.isSetter() || currentEntry.isMethod()` necessary? No, removed. >> Source/JavaScriptCore/parser/VariableEnvironment.cpp:173 >> + } > > Looks like declarePrivateGetter is largely duplicate to declarePrivateSetter. Could you dedupe the code? Done. >> Source/JavaScriptCore/parser/VariableEnvironment.cpp:177 >> + if (currentEntry.isGetter() || currentEntry.isMethod() || !currentEntry.isSetter()) > > Is `currentEntry.isGetter() || currentEntry.isMethod()` necessary? Removed
Created attachment 420350 [details] Patch
Committed r272883: <https://commits.webkit.org/r272883> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420350 [details].
<rdar://problem/74366369>