From: Benjamin Kramer Date: Wed, 21 Oct 2015 10:07:26 +0000 (+0000) Subject: Revert "[AST] Put TypeLocs and NestedNameSpecifierLocs into the ParentMap." X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=7106dc1921dacfb74d47482fb876903d18332324;p=clang Revert "[AST] Put TypeLocs and NestedNameSpecifierLocs into the ParentMap." Putting DynTypedNode in the ParentMap bloats its memory foot print. Before the void* key had 8 bytes, now we're at 40 bytes per key which can mean multiple gigabytes increase for large ASTs and this count doesn't even include all the added TypeLoc nodes. Revert until I come up with a better data structure. This reverts commit r250831. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@250889 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/AST/ASTContext.h b/include/clang/AST/ASTContext.h index 7c7b910077..5b8240925d 100644 --- a/include/clang/AST/ASTContext.h +++ b/include/clang/AST/ASTContext.h @@ -452,7 +452,7 @@ public: typedef llvm::SmallVector ParentVector; /// \brief Maps from a node to its parents. - typedef llvm::DenseMap> ParentMap; diff --git a/include/clang/AST/ASTTypeTraits.h b/include/clang/AST/ASTTypeTraits.h index f5274d5043..dc3c34f28d 100644 --- a/include/clang/AST/ASTTypeTraits.h +++ b/include/clang/AST/ASTTypeTraits.h @@ -253,30 +253,10 @@ public: /// @{ /// \brief Imposes an order on \c DynTypedNode. /// - /// FIXME: Implement comparsion for other node types. + /// Supports comparison of nodes that support memoization. + /// FIXME: Implement comparsion for other node types (currently + /// only Stmt, Decl, Type and NestedNameSpecifier return memoization data). bool operator<(const DynTypedNode &Other) const { - if (!NodeKind.isSame(Other.NodeKind)) - return NodeKind < Other.NodeKind; - - if (ASTNodeKind::getFromNodeKind().isSame(NodeKind)) { - auto TLA = getUnchecked(); - auto TLB = Other.getUnchecked(); - return std::make_pair(TLA.getType().getAsOpaquePtr(), - TLA.getOpaqueData()) < - std::make_pair(TLB.getType().getAsOpaquePtr(), - TLB.getOpaqueData()); - } - - if (ASTNodeKind::getFromNodeKind().isSame( - NodeKind)) { - auto NNSLA = getUnchecked(); - auto NNSLB = Other.getUnchecked(); - return std::make_pair(NNSLA.getNestedNameSpecifier(), - NNSLA.getOpaqueData()) < - std::make_pair(NNSLB.getNestedNameSpecifier(), - NNSLB.getOpaqueData()); - } - assert(getMemoizationData() && Other.getMemoizationData()); return getMemoizationData() < Other.getMemoizationData(); } @@ -290,13 +270,6 @@ public: if (ASTNodeKind::getFromNodeKind().isSame(NodeKind)) return getUnchecked() == Other.getUnchecked(); - if (ASTNodeKind::getFromNodeKind().isSame(NodeKind)) - return getUnchecked() == Other.getUnchecked(); - - if (ASTNodeKind::getFromNodeKind().isSame(NodeKind)) - return getUnchecked() == - Other.getUnchecked(); - assert(getMemoizationData() && Other.getMemoizationData()); return getMemoizationData() == Other.getMemoizationData(); } @@ -305,47 +278,6 @@ public: } /// @} - /// \brief Hooks for using DynTypedNode as a key in a DenseMap. - struct DenseMapInfo { - static inline DynTypedNode getEmptyKey() { - DynTypedNode Node; - Node.NodeKind = ASTNodeKind::DenseMapInfo::getEmptyKey(); - return Node; - } - static inline DynTypedNode getTombstoneKey() { - DynTypedNode Node; - Node.NodeKind = ASTNodeKind::DenseMapInfo::getTombstoneKey(); - return Node; - } - static unsigned getHashValue(const DynTypedNode &Val) { - // FIXME: Add hashing support for the remaining types. - if (ASTNodeKind::getFromNodeKind().isSame(Val.NodeKind)) { - auto TL = Val.getUnchecked(); - return llvm::hash_combine(TL.getType().getAsOpaquePtr(), - TL.getOpaqueData()); - } - - if (ASTNodeKind::getFromNodeKind().isSame( - Val.NodeKind)) { - auto NNSL = Val.getUnchecked(); - return llvm::hash_combine(NNSL.getNestedNameSpecifier(), - NNSL.getOpaqueData()); - } - - assert(Val.getMemoizationData()); - return llvm::hash_value(Val.getMemoizationData()); - } - static bool isEqual(const DynTypedNode &LHS, const DynTypedNode &RHS) { - auto Empty = ASTNodeKind::DenseMapInfo::getEmptyKey(); - auto TombStone = ASTNodeKind::DenseMapInfo::getTombstoneKey(); - return (ASTNodeKind::DenseMapInfo::isEqual(LHS.NodeKind, Empty) && - ASTNodeKind::DenseMapInfo::isEqual(RHS.NodeKind, Empty)) || - (ASTNodeKind::DenseMapInfo::isEqual(LHS.NodeKind, TombStone) && - ASTNodeKind::DenseMapInfo::isEqual(RHS.NodeKind, TombStone)) || - LHS == RHS; - } - }; - private: /// \brief Takes care of converting from and to \c T. template struct BaseConverter; @@ -488,10 +420,6 @@ template <> struct DenseMapInfo : clang::ast_type_traits::ASTNodeKind::DenseMapInfo {}; -template <> -struct DenseMapInfo - : clang::ast_type_traits::DynTypedNode::DenseMapInfo {}; - } // end namespace llvm #endif diff --git a/include/clang/ASTMatchers/ASTMatchers.h b/include/clang/ASTMatchers/ASTMatchers.h index 0a1800b9d4..484cc943ea 100644 --- a/include/clang/ASTMatchers/ASTMatchers.h +++ b/include/clang/ASTMatchers/ASTMatchers.h @@ -2068,10 +2068,8 @@ internal::Matcher findAll(const internal::Matcher &Matcher) { /// /// Usable as: Any Matcher const internal::ArgumentAdaptingMatcherFunc< - internal::HasParentMatcher, - internal::TypeList, - internal::TypeList> - LLVM_ATTRIBUTE_UNUSED hasParent = {}; + internal::HasParentMatcher, internal::TypeList, + internal::TypeList > LLVM_ATTRIBUTE_UNUSED hasParent = {}; /// \brief Matches AST nodes that have an ancestor that matches the provided /// matcher. @@ -2085,10 +2083,8 @@ const internal::ArgumentAdaptingMatcherFunc< /// /// Usable as: Any Matcher const internal::ArgumentAdaptingMatcherFunc< - internal::HasAncestorMatcher, - internal::TypeList, - internal::TypeList> - LLVM_ATTRIBUTE_UNUSED hasAncestor = {}; + internal::HasAncestorMatcher, internal::TypeList, + internal::TypeList > LLVM_ATTRIBUTE_UNUSED hasAncestor = {}; /// \brief Matches if the provided matcher does not match. /// diff --git a/include/clang/ASTMatchers/ASTMatchersInternal.h b/include/clang/ASTMatchers/ASTMatchersInternal.h index 55839c53b4..8e1abc6f77 100644 --- a/include/clang/ASTMatchers/ASTMatchersInternal.h +++ b/include/clang/ASTMatchers/ASTMatchersInternal.h @@ -848,10 +848,8 @@ public: BoundNodesTreeBuilder *Builder, AncestorMatchMode MatchMode) { static_assert(std::is_base_of::value || - std::is_base_of::value || - std::is_base_of::value || - std::is_base_of::value, - "type not allowed for recursive matching"); + std::is_base_of::value, + "only Decl or Stmt allowed for recursive matching"); return matchesAncestorOf(ast_type_traits::DynTypedNode::create(Node), Matcher, Builder, MatchMode); } diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp index c26d772139..848158877a 100644 --- a/lib/AST/ASTContext.cpp +++ b/lib/AST/ASTContext.cpp @@ -8678,23 +8678,6 @@ bool ASTContext::AtomicUsesUnsupportedLibcall(const AtomicExpr *E) const { namespace { -/// Template specializations to abstract away from pointers and TypeLocs. -/// @{ -template -ast_type_traits::DynTypedNode createDynTypedNode(const T &Node) { - return ast_type_traits::DynTypedNode::create(*Node); -} -template <> -ast_type_traits::DynTypedNode createDynTypedNode(const TypeLoc &Node) { - return ast_type_traits::DynTypedNode::create(Node); -} -template <> -ast_type_traits::DynTypedNode -createDynTypedNode(const NestedNameSpecifierLoc &Node) { - return ast_type_traits::DynTypedNode::create(Node); -} -/// @} - /// \brief A \c RecursiveASTVisitor that builds a map from nodes to their /// parents as defined by the \c RecursiveASTVisitor. /// @@ -8702,8 +8685,7 @@ createDynTypedNode(const NestedNameSpecifierLoc &Node) { /// traversal - there are other relationships (for example declaration context) /// in the AST that are better modeled by special matchers. /// -/// FIXME: Currently only builds up the map using \c Stmt, \c Decl, -/// \c NestedNameSpecifierLoc and \c TypeLoc nodes. + /// FIXME: Currently only builds up the map using \c Stmt and \c Decl nodes. class ParentMapASTVisitor : public RecursiveASTVisitor { public: @@ -8735,11 +8717,21 @@ createDynTypedNode(const NestedNameSpecifierLoc &Node) { } template - bool TraverseNode(T Node, bool (VisitorBase::*traverse)(T)) { + bool TraverseNode(T *Node, bool(VisitorBase:: *traverse) (T *)) { if (!Node) return true; if (ParentStack.size() > 0) { - auto &NodeOrVector = (*Parents)[createDynTypedNode(Node)]; + // FIXME: Currently we add the same parent multiple times, but only + // when no memoization data is available for the type. + // For example when we visit all subexpressions of template + // instantiations; this is suboptimal, but benign: the only way to + // visit those is with hasAncestor / hasParent, and those do not create + // new matches. + // The plan is to enable DynTypedNode to be storable in a map or hash + // map. The main problem there is to implement hash functions / + // comparison operators for all types that DynTypedNode supports that + // do not have pointer identity. + auto &NodeOrVector = (*Parents)[Node]; if (NodeOrVector.isNull()) { NodeOrVector = new ast_type_traits::DynTypedNode(ParentStack.back()); } else { @@ -8765,7 +8757,7 @@ createDynTypedNode(const NestedNameSpecifierLoc &Node) { Vector->push_back(ParentStack.back()); } } - ParentStack.push_back(createDynTypedNode(Node)); + ParentStack.push_back(ast_type_traits::DynTypedNode::create(*Node)); bool Result = (this ->* traverse) (Node); ParentStack.pop_back(); return Result; @@ -8779,15 +8771,6 @@ createDynTypedNode(const NestedNameSpecifierLoc &Node) { return TraverseNode(StmtNode, &VisitorBase::TraverseStmt); } - bool TraverseTypeLoc(TypeLoc TypeLocNode) { - return TraverseNode(TypeLocNode, &VisitorBase::TraverseTypeLoc); - } - - bool TraverseNestedNameSpecifierLoc(NestedNameSpecifierLoc NNSLocNode) { - return TraverseNode(NNSLocNode, - &VisitorBase::TraverseNestedNameSpecifierLoc); - } - ASTContext::ParentMap *Parents; llvm::SmallVector ParentStack; @@ -8798,13 +8781,16 @@ createDynTypedNode(const NestedNameSpecifierLoc &Node) { ArrayRef ASTContext::getParents(const ast_type_traits::DynTypedNode &Node) { + assert(Node.getMemoizationData() && + "Invariant broken: only nodes that support memoization may be " + "used in the parent map."); if (!AllParents) { // We always need to run over the whole translation unit, as // hasAncestor can escape any subtree. AllParents.reset( ParentMapASTVisitor::buildMap(*getTranslationUnitDecl())); } - ParentMap::const_iterator I = AllParents->find(Node); + ParentMap::const_iterator I = AllParents->find(Node.getMemoizationData()); if (I == AllParents->end()) { return None; } diff --git a/lib/ASTMatchers/ASTMatchFinder.cpp b/lib/ASTMatchers/ASTMatchFinder.cpp index 571e7dac24..8807b13c5e 100644 --- a/lib/ASTMatchers/ASTMatchFinder.cpp +++ b/lib/ASTMatchers/ASTMatchFinder.cpp @@ -621,6 +621,9 @@ private: if (Node.get() == ActiveASTContext->getTranslationUnitDecl()) return false; + assert(Node.getMemoizationData() && + "Invariant broken: only nodes that support memoization may be " + "used in the parent map."); MatchKey Key; Key.MatcherID = Matcher.getID(); @@ -864,11 +867,7 @@ bool MatchASTVisitor::TraverseNestedNameSpecifier(NestedNameSpecifier *NNS) { bool MatchASTVisitor::TraverseNestedNameSpecifierLoc( NestedNameSpecifierLoc NNS) { - if (!NNS) - return true; - match(NNS); - // We only match the nested name specifier here (as opposed to traversing it) // because the traversal is already done in the parallel "Loc"-hierarchy. if (NNS.hasQualifier()) diff --git a/unittests/AST/ASTContextParentMapTest.cpp b/unittests/AST/ASTContextParentMapTest.cpp index b1d7db4164..94e9735654 100644 --- a/unittests/AST/ASTContextParentMapTest.cpp +++ b/unittests/AST/ASTContextParentMapTest.cpp @@ -38,19 +38,6 @@ TEST(GetParents, ReturnsParentForStmt) { ifStmt(hasParent(compoundStmt())))); } -TEST(GetParents, ReturnsParentForTypeLoc) { - MatchVerifier Verifier; - EXPECT_TRUE( - Verifier.match("namespace a { class b {}; } void f(a::b) {}", - typeLoc(hasParent(typeLoc(hasParent(functionDecl())))))); -} - -TEST(GetParents, ReturnsParentForNestedNameSpecifierLoc) { - MatchVerifier Verifier; - EXPECT_TRUE(Verifier.match("namespace a { class b {}; } void f(a::b) {}", - nestedNameSpecifierLoc(hasParent(typeLoc())))); -} - TEST(GetParents, ReturnsParentInsideTemplateInstantiations) { MatchVerifier DeclVerifier; EXPECT_TRUE(DeclVerifier.match( diff --git a/unittests/ASTMatchers/Dynamic/ParserTest.cpp b/unittests/ASTMatchers/Dynamic/ParserTest.cpp index 2c94a727de..ef66a816aa 100644 --- a/unittests/ASTMatchers/Dynamic/ParserTest.cpp +++ b/unittests/ASTMatchers/Dynamic/ParserTest.cpp @@ -318,8 +318,7 @@ TEST(ParserTest, CompletionNamedValues) { Comps[1].MatcherDecl); EXPECT_EQ("arent(", Comps[2].TypedText); - EXPECT_EQ("Matcher " - "hasParent(Matcher)", + EXPECT_EQ("Matcher hasParent(Matcher)", Comps[2].MatcherDecl); } diff --git a/unittests/ASTMatchers/Dynamic/RegistryTest.cpp b/unittests/ASTMatchers/Dynamic/RegistryTest.cpp index 9b562bcc9d..ca1dfefd53 100644 --- a/unittests/ASTMatchers/Dynamic/RegistryTest.cpp +++ b/unittests/ASTMatchers/Dynamic/RegistryTest.cpp @@ -447,10 +447,8 @@ TEST_F(RegistryTest, Errors) { TEST_F(RegistryTest, Completion) { CompVector Comps = getCompletions(); // Overloaded - EXPECT_TRUE(hasCompletion(Comps, "hasParent(", - "Matcher " - "hasParent(Matcher)")); + EXPECT_TRUE(hasCompletion( + Comps, "hasParent(", "Matcher hasParent(Matcher)")); // Variadic. EXPECT_TRUE(hasCompletion(Comps, "whileStmt(", "Matcher whileStmt(Matcher...)")); @@ -465,10 +463,8 @@ TEST_F(RegistryTest, Completion) { EXPECT_TRUE(hasCompletion(WhileComps, "hasBody(", "Matcher hasBody(Matcher)")); - EXPECT_TRUE(hasCompletion(WhileComps, "hasParent(", "Matcher " - "hasParent(Matcher<" - "NestedNameSpecifierLoc|" - "TypeLoc|Decl|...>)")); + EXPECT_TRUE(hasCompletion(WhileComps, "hasParent(", + "Matcher hasParent(Matcher)")); EXPECT_TRUE( hasCompletion(WhileComps, "allOf(", "Matcher allOf(Matcher...)"));