Bug 219181

<rdar://problem/71754440>

Created attachment 420525 [details] Patch

Created attachment 420629 [details] Patch

Comment on attachment 420629 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420629&action=review r=me with comments. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2817 > + RefPtr<RegisterID> condition = newTemporary(); > + emitEqualityOp<OpStricteq>(condition.get(), base, brand); > + emitJumpIfTrue(condition.get(), brandCheckOkLabel.get()); Please follow https://bugs.webkit.org/show_bug.cgi?id=221927 style > Source/JavaScriptCore/parser/Parser.cpp:3059 > + auto declarationResult = classScope->declarePrivateSetter(*ident, tag); > + semanticFailIfTrue(declarationResult & DeclarationResult::InvalidDuplicateDeclaration, "Declared private setter with an already used name"); > + if (tag == ClassElementTag::Static) { > + semanticFailIfTrue(declarationResult & DeclarationResult::InvalidPrivateStaticNonStatic, "Cannot declare a private static setter if there is a non-static private getter with used name"); > + declaresStaticPrivateAccessor = true; > + } else { > + semanticFailIfTrue(declarationResult & DeclarationResult::InvalidPrivateStaticNonStatic, "Cannot declare a private non-static setter if there is a static private getter with used name"); > + declaresPrivateAccessor = true; > + } > type = static_cast<PropertyNode::Type>(type | PropertyNode::PrivateSetter); > } else { > - semanticFailIfTrue(classScope->declarePrivateGetter(*ident) & DeclarationResult::InvalidDuplicateDeclaration, "Declared private getter with an already used name"); > - declaresPrivateAccessor = true; > + auto declarationResult = classScope->declarePrivateGetter(*ident, tag); > + semanticFailIfTrue(declarationResult & DeclarationResult::InvalidDuplicateDeclaration, "Declared private getter with an already used name"); > + if (tag == ClassElementTag::Static) { > + semanticFailIfTrue(declarationResult & DeclarationResult::InvalidPrivateStaticNonStatic, "Cannot declare a private static getter if there is a non-static private setter with used name"); > + declaresStaticPrivateAccessor = true; > + } else { > + semanticFailIfTrue(declarationResult & DeclarationResult::InvalidPrivateStaticNonStatic, "Cannot declare a private non-static getter if there is a static private setter with used name"); > + declaresPrivateAccessor = true; > + } Both looks almost identical. We should remove `if (isSetter)` branching and pass "getter" / "setter" as a parameter to the error message. auto declarationResult = classScope->declarePrivateGetter(*ident, tag); semanticFailIfTrue(declarationResult & DeclarationResult::InvalidDuplicateDeclaration, "Declared private ", (isSetter ? "setter" : "getter"), " with an already used name"); if (tag == ClassElementTag::Static) { semanticFailIfTrue(declarationResult & DeclarationResult::InvalidPrivateStaticNonStatic, "Cannot declare a private static ", (isSetter ? "setter" : "getter"), "if there is a non-static private setter with used name"); declaresStaticPrivateAccessor = true; } else { semanticFailIfTrue(declarationResult & DeclarationResult::InvalidPrivateStaticNonStatic, "Cannot declare a private non-static ", (isSetter ? "setter" : "getter"), " if there is a static private setter with used name"); declaresPrivateAccessor = true; } > Source/JavaScriptCore/parser/Parser.h:518 > return result; Can you explain why declarePrivateMethod requires useVariable(&ident, false); addClosedVariableCandidateUnconditionally(ident.impl()); and the others are not? > Source/JavaScriptCore/parser/Parser.h:555 > + DeclarationResultMask declarePrivateSetter(const Identifier& ident, ClassElementTag tag) > { > ASSERT(m_allowsLexicalDeclarations); > DeclarationResultMask result = DeclarationResult::Valid; > - bool addResult = m_lexicalVariables.declarePrivateSetter(ident); > + auto addResult = tag == ClassElementTag::Static ? m_lexicalVariables.declareStaticPrivateSetter(ident) : m_lexicalVariables.declarePrivateSetter(ident); > > - if (!addResult) { > + if (addResult == VariableEnvironment::PrivateDeclarationResult::DuplicatedName) { > result |= DeclarationResult::InvalidDuplicateDeclaration; > return result; > } > > + if (addResult == VariableEnvironment::PrivateDeclarationResult::InvalidStaticNonStatic) { > + result |= DeclarationResult::InvalidPrivateStaticNonStatic; > + return result; > + } > + > return result; > } > > - DeclarationResultMask declarePrivateGetter(const Identifier& ident) > + DeclarationResultMask declarePrivateGetter(const Identifier& ident, ClassElementTag tag) > { > ASSERT(m_allowsLexicalDeclarations); > DeclarationResultMask result = DeclarationResult::Valid; > - bool addResult = m_lexicalVariables.declarePrivateGetter(ident); > + auto addResult = tag == ClassElementTag::Static ? m_lexicalVariables.declareStaticPrivateGetter(ident) : m_lexicalVariables.declarePrivateGetter(ident); > > - if (!addResult) { > + if (addResult == VariableEnvironment::PrivateDeclarationResult::DuplicatedName) { > result |= DeclarationResult::InvalidDuplicateDeclaration; > return result; > } > > + if (addResult == VariableEnvironment::PrivateDeclarationResult::InvalidStaticNonStatic) { > + result |= DeclarationResult::InvalidPrivateStaticNonStatic; > + return result; > + } > + These functions are almost identical. Can you extract the common part out from them?

Comment on attachment 420629 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420629&action=review Thanks a lot for the review >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2817 >> + emitJumpIfTrue(condition.get(), brandCheckOkLabel.get()); > > Please follow https://bugs.webkit.org/show_bug.cgi?id=221927 style Done. >> Source/JavaScriptCore/parser/Parser.cpp:3059 >> + } > > Both looks almost identical. We should remove `if (isSetter)` branching and pass "getter" / "setter" as a parameter to the error message. > > auto declarationResult = classScope->declarePrivateGetter(*ident, tag); > semanticFailIfTrue(declarationResult & DeclarationResult::InvalidDuplicateDeclaration, "Declared private ", (isSetter ? "setter" : "getter"), " with an already used name"); > if (tag == ClassElementTag::Static) { > semanticFailIfTrue(declarationResult & DeclarationResult::InvalidPrivateStaticNonStatic, "Cannot declare a private static ", (isSetter ? "setter" : "getter"), "if there is a non-static private setter with used name"); > declaresStaticPrivateAccessor = true; > } else { > semanticFailIfTrue(declarationResult & DeclarationResult::InvalidPrivateStaticNonStatic, "Cannot declare a private non-static ", (isSetter ? "setter" : "getter"), " if there is a static private setter with used name"); > declaresPrivateAccessor = true; > } Done. I needed to add a couple of ternary per message, but we then removed a lot of duplicated code. >> Source/JavaScriptCore/parser/Parser.h:518 >> return result; > > Can you explain why declarePrivateMethod requires > > useVariable(&ident, false); > addClosedVariableCandidateUnconditionally(ident.impl()); > > and the others are not? This is a leftover from an old implementation of `VariableEnvironment::declarePrivateMethod` that was not setting the `VariableEnvyronmentEntry` properly. I just removed it, since it's unnecessary. >> Source/JavaScriptCore/parser/Parser.h:555 >> + > > These functions are almost identical. Can you extract the common part out from them? Done.

Created attachment 420828 [details] Patch

Committed r273107: <https://commits.webkit.org/r273107> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420828 [details].