[Clang] [NFC] Refactor more AST visitors #116823

Sirraide · 2024-11-19T15:25:01Z

This pr migrates most of the remaining AST visitors all across Clang to use DynamicRecursiveASTVisitor.

This is part of an ongoing refactor. For more context, see #110040, #93462.

llvmbot · 2024-11-19T15:25:40Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-debuginfo

Author: None (Sirraide)

Changes

This pr migrates most of the remaining AST visitors all across Clang to use DynamicRecursiveASTVisitor.

This is part of an ongoing refactor. For more context, see #110040, #93462.

Patch is 89.74 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/116823.diff

18 Files Affected:

(modified) clang/include/clang/InstallAPI/Visitor.h (+11-9)
(modified) clang/include/clang/Tooling/Refactoring/RecursiveSymbolVisitor.h (+15-20)
(modified) clang/lib/AST/ASTImporterLookupTable.cpp (+10-10)
(modified) clang/lib/AST/ParentMapContext.cpp (+30-23)
(modified) clang/lib/ASTMatchers/ASTMatchFinder.cpp (+84-94)
(modified) clang/lib/CodeGen/CGDebugInfo.cpp (+10-11)
(modified) clang/lib/CodeGen/CodeGenModule.cpp (+10-11)
(modified) clang/lib/CodeGen/CodeGenPGO.cpp (+25-27)
(modified) clang/lib/CodeGen/ObjectFilePCHContainerWriter.cpp (+7-7)
(modified) clang/lib/Frontend/ASTConsumers.cpp (+110-113)
(modified) clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp (+3-3)
(modified) clang/lib/Index/IndexBody.cpp (+43-43)
(modified) clang/lib/Index/IndexTypeSourceInfo.cpp (+22-21)
(modified) clang/lib/InstallAPI/Visitor.cpp (+5-5)
(modified) clang/lib/Tooling/ASTDiff/ASTDiff.cpp (+11-10)
(modified) clang/lib/Tooling/Refactoring/Rename/USRFinder.cpp (+4-7)
(modified) clang/lib/Tooling/Refactoring/Rename/USRFindingAction.cpp (+6-6)
(modified) clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp (+12-13)

diff --git a/clang/include/clang/InstallAPI/Visitor.h b/clang/include/clang/InstallAPI/Visitor.h
index 3680ee566ca875..a04100b7b147b3 100644
--- a/clang/include/clang/InstallAPI/Visitor.h
+++ b/clang/include/clang/InstallAPI/Visitor.h
@@ -13,8 +13,8 @@
 #ifndef LLVM_CLANG_INSTALLAPI_VISITOR_H
 #define LLVM_CLANG_INSTALLAPI_VISITOR_H
 
+#include "clang/AST/DynamicRecursiveASTVisitor.h"
 #include "clang/AST/Mangle.h"
-#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/InstallAPI/Context.h"
@@ -26,35 +26,37 @@ namespace installapi {
 
 /// ASTVisitor for collecting declarations that represent global symbols.
 class InstallAPIVisitor final : public ASTConsumer,
-                                public RecursiveASTVisitor<InstallAPIVisitor> {
+                                public DynamicRecursiveASTVisitor {
 public:
   InstallAPIVisitor(ASTContext &ASTCtx, InstallAPIContext &Ctx,
                     SourceManager &SrcMgr, Preprocessor &PP)
       : Ctx(Ctx), SrcMgr(SrcMgr), PP(PP),
         MC(ItaniumMangleContext::create(ASTCtx, ASTCtx.getDiagnostics())),
-        Layout(ASTCtx.getTargetInfo().getDataLayoutString()) {}
+        Layout(ASTCtx.getTargetInfo().getDataLayoutString()) {
+    ShouldVisitTemplateInstantiations = true;
+  }
+
   void HandleTranslationUnit(ASTContext &ASTCtx) override;
-  bool shouldVisitTemplateInstantiations() const { return true; }
 
   /// Collect global variables.
-  bool VisitVarDecl(const VarDecl *D);
+  bool VisitVarDecl(VarDecl *D) override;
 
   /// Collect global functions.
-  bool VisitFunctionDecl(const FunctionDecl *D);
+  bool VisitFunctionDecl(FunctionDecl *D) override;
 
   /// Collect Objective-C Interface declarations.
   /// Every Objective-C class has an interface declaration that lists all the
   /// ivars, properties, and methods of the class.
-  bool VisitObjCInterfaceDecl(const ObjCInterfaceDecl *D);
+  bool VisitObjCInterfaceDecl(ObjCInterfaceDecl *D) override;
 
   /// Collect Objective-C Category/Extension declarations.
   ///
   /// The class that is being extended might come from a different library and
   /// is therefore itself not collected.
-  bool VisitObjCCategoryDecl(const ObjCCategoryDecl *D);
+  bool VisitObjCCategoryDecl(ObjCCategoryDecl *D) override;
 
   /// Collect global c++ declarations.
-  bool VisitCXXRecordDecl(const CXXRecordDecl *D);
+  bool VisitCXXRecordDecl(CXXRecordDecl *D) override;
 
 private:
   std::string getMangledName(const NamedDecl *D) const;
diff --git a/clang/include/clang/Tooling/Refactoring/RecursiveSymbolVisitor.h b/clang/include/clang/Tooling/Refactoring/RecursiveSymbolVisitor.h
index 015dbba26f6887..ab1d770046d827 100644
--- a/clang/include/clang/Tooling/Refactoring/RecursiveSymbolVisitor.h
+++ b/clang/include/clang/Tooling/Refactoring/RecursiveSymbolVisitor.h
@@ -16,7 +16,7 @@
 #define LLVM_CLANG_TOOLING_REFACTORING_RECURSIVESYMBOLVISITOR_H
 
 #include "clang/AST/AST.h"
-#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/DynamicRecursiveASTVisitor.h"
 #include "clang/Lex/Lexer.h"
 
 namespace clang {
@@ -24,27 +24,23 @@ namespace tooling {
 
 /// Traverses the AST and visits the occurrence of each named symbol in the
 /// given nodes.
-template <typename T>
-class RecursiveSymbolVisitor
-    : public RecursiveASTVisitor<RecursiveSymbolVisitor<T>> {
-  using BaseType = RecursiveASTVisitor<RecursiveSymbolVisitor<T>>;
-
+class RecursiveSymbolVisitor : public DynamicRecursiveASTVisitor {
 public:
   RecursiveSymbolVisitor(const SourceManager &SM, const LangOptions &LangOpts)
       : SM(SM), LangOpts(LangOpts) {}
 
-  bool visitSymbolOccurrence(const NamedDecl *ND,
-                             ArrayRef<SourceRange> NameRanges) {
+  virtual bool visitSymbolOccurrence(const NamedDecl *ND,
+                                     ArrayRef<SourceRange> NameRanges) {
     return true;
   }
 
   // Declaration visitors:
 
-  bool VisitNamedDecl(const NamedDecl *D) {
+  bool VisitNamedDecl(NamedDecl *D) override {
     return isa<CXXConversionDecl>(D) ? true : visit(D, D->getLocation());
   }
 
-  bool VisitCXXConstructorDecl(const CXXConstructorDecl *CD) {
+  bool VisitCXXConstructorDecl(CXXConstructorDecl *CD) override {
     for (const auto *Initializer : CD->inits()) {
       // Ignore implicit initializers.
       if (!Initializer->isWritten())
@@ -61,15 +57,15 @@ class RecursiveSymbolVisitor
 
   // Expression visitors:
 
-  bool VisitDeclRefExpr(const DeclRefExpr *Expr) {
+  bool VisitDeclRefExpr(DeclRefExpr *Expr) override {
     return visit(Expr->getFoundDecl(), Expr->getLocation());
   }
 
-  bool VisitMemberExpr(const MemberExpr *Expr) {
+  bool VisitMemberExpr(MemberExpr *Expr) override {
     return visit(Expr->getFoundDecl().getDecl(), Expr->getMemberLoc());
   }
 
-  bool VisitOffsetOfExpr(const OffsetOfExpr *S) {
+  bool VisitOffsetOfExpr(OffsetOfExpr *S) override {
     for (unsigned I = 0, E = S->getNumComponents(); I != E; ++I) {
       const OffsetOfNode &Component = S->getComponent(I);
       if (Component.getKind() == OffsetOfNode::Field) {
@@ -83,7 +79,7 @@ class RecursiveSymbolVisitor
 
   // Other visitors:
 
-  bool VisitTypeLoc(const TypeLoc Loc) {
+  bool VisitTypeLoc(TypeLoc Loc) override {
     const SourceLocation TypeBeginLoc = Loc.getBeginLoc();
     const SourceLocation TypeEndLoc =
         Lexer::getLocForEndOfToken(TypeBeginLoc, 0, SM, LangOpts);
@@ -105,13 +101,13 @@ class RecursiveSymbolVisitor
     return true;
   }
 
-  bool VisitTypedefTypeLoc(TypedefTypeLoc TL) {
+  bool VisitTypedefTypeLoc(TypedefTypeLoc TL) override {
     const SourceLocation TypeEndLoc =
         Lexer::getLocForEndOfToken(TL.getBeginLoc(), 0, SM, LangOpts);
     return visit(TL.getTypedefNameDecl(), TL.getBeginLoc(), TypeEndLoc);
   }
 
-  bool TraverseNestedNameSpecifierLoc(NestedNameSpecifierLoc NNS) {
+  bool TraverseNestedNameSpecifierLoc(NestedNameSpecifierLoc NNS) override {
     // The base visitor will visit NNSL prefixes, so we should only look at
     // the current NNS.
     if (NNS) {
@@ -119,10 +115,10 @@ class RecursiveSymbolVisitor
       if (!visit(ND, NNS.getLocalBeginLoc(), NNS.getLocalEndLoc()))
         return false;
     }
-    return BaseType::TraverseNestedNameSpecifierLoc(NNS);
+    return DynamicRecursiveASTVisitor::TraverseNestedNameSpecifierLoc(NNS);
   }
 
-  bool VisitDesignatedInitExpr(const DesignatedInitExpr *E) {
+  bool VisitDesignatedInitExpr(DesignatedInitExpr *E) override {
     for (const DesignatedInitExpr::Designator &D : E->designators()) {
       if (D.isFieldDesignator()) {
         if (const FieldDecl *Decl = D.getFieldDecl()) {
@@ -140,8 +136,7 @@ class RecursiveSymbolVisitor
 
   bool visit(const NamedDecl *ND, SourceLocation BeginLoc,
              SourceLocation EndLoc) {
-    return static_cast<T *>(this)->visitSymbolOccurrence(
-        ND, SourceRange(BeginLoc, EndLoc));
+    return visitSymbolOccurrence(ND, SourceRange(BeginLoc, EndLoc));
   }
   bool visit(const NamedDecl *ND, SourceLocation Loc) {
     return visit(ND, Loc, Lexer::getLocForEndOfToken(Loc, 0, SM, LangOpts));
diff --git a/clang/lib/AST/ASTImporterLookupTable.cpp b/clang/lib/AST/ASTImporterLookupTable.cpp
index 07d39dcee2583a..b7c112540c1c60 100644
--- a/clang/lib/AST/ASTImporterLookupTable.cpp
+++ b/clang/lib/AST/ASTImporterLookupTable.cpp
@@ -13,18 +13,22 @@
 
 #include "clang/AST/ASTImporterLookupTable.h"
 #include "clang/AST/Decl.h"
-#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/DeclFriend.h"
+#include "clang/AST/DynamicRecursiveASTVisitor.h"
 #include "llvm/Support/FormatVariadic.h"
 
 namespace clang {
 
 namespace {
 
-struct Builder : RecursiveASTVisitor<Builder> {
+struct Builder : DynamicRecursiveASTVisitor {
   ASTImporterLookupTable &LT;
-  Builder(ASTImporterLookupTable &LT) : LT(LT) {}
+  Builder(ASTImporterLookupTable &LT) : LT(LT) {
+    ShouldVisitTemplateInstantiations = true;
+    ShouldVisitImplicitCode = true;
+  }
 
-  bool VisitTypedefNameDecl(TypedefNameDecl *D) {
+  bool VisitTypedefNameDecl(TypedefNameDecl *D) override {
     QualType Ty = D->getUnderlyingType();
     Ty = Ty.getCanonicalType();
     if (const auto *RTy = dyn_cast<RecordType>(Ty)) {
@@ -37,7 +41,7 @@ struct Builder : RecursiveASTVisitor<Builder> {
     return true;
   }
 
-  bool VisitNamedDecl(NamedDecl *D) {
+  bool VisitNamedDecl(NamedDecl *D) override {
     LT.add(D);
     return true;
   }
@@ -46,7 +50,7 @@ struct Builder : RecursiveASTVisitor<Builder> {
   // visitation. However, there are cases when the befriended class is not a
   // child, thus it must be fetched explicitly from the FriendDecl, and only
   // then can we add it to the lookup table.
-  bool VisitFriendDecl(FriendDecl *D) {
+  bool VisitFriendDecl(FriendDecl *D) override {
     if (D->getFriendType()) {
       QualType Ty = D->getFriendType()->getType();
       if (isa<ElaboratedType>(Ty))
@@ -76,10 +80,6 @@ struct Builder : RecursiveASTVisitor<Builder> {
     }
     return true;
   }
-
-  // Override default settings of base.
-  bool shouldVisitTemplateInstantiations() const { return true; }
-  bool shouldVisitImplicitCode() const { return true; }
 };
 
 } // anonymous namespace
diff --git a/clang/lib/AST/ParentMapContext.cpp b/clang/lib/AST/ParentMapContext.cpp
index 9723c0cfa83bbe..a5420f28750956 100644
--- a/clang/lib/AST/ParentMapContext.cpp
+++ b/clang/lib/AST/ParentMapContext.cpp
@@ -12,9 +12,10 @@
 //===----------------------------------------------------------------------===//
 
 #include "clang/AST/ParentMapContext.h"
-#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/DynamicRecursiveASTVisitor.h"
 #include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
 #include "clang/AST/TemplateBase.h"
 
 using namespace clang;
@@ -352,19 +353,14 @@ template <> DynTypedNode createDynTypedNode(const ObjCProtocolLoc &Node) {
 /// traversal - there are other relationships (for example declaration context)
 /// in the AST that are better modeled by special matchers.
 class ParentMapContext::ParentMap::ASTVisitor
-    : public RecursiveASTVisitor<ASTVisitor> {
+    : public DynamicRecursiveASTVisitor {
 public:
-  ASTVisitor(ParentMap &Map) : Map(Map) {}
+  ASTVisitor(ParentMap &Map) : Map(Map) {
+    ShouldVisitTemplateInstantiations = true;
+    ShouldVisitImplicitCode = true;
+  }
 
 private:
-  friend class RecursiveASTVisitor<ASTVisitor>;
-
-  using VisitorBase = RecursiveASTVisitor<ASTVisitor>;
-
-  bool shouldVisitTemplateInstantiations() const { return true; }
-
-  bool shouldVisitImplicitCode() const { return true; }
-
   /// Record the parent of the node we're visiting.
   /// MapNode is the child, the parent is on top of ParentStack.
   /// Parents is the parent storage (either PointerParents or OtherParents).
@@ -427,42 +423,53 @@ class ParentMapContext::ParentMap::ASTVisitor
     return Result;
   }
 
-  bool TraverseDecl(Decl *DeclNode) {
+  bool TraverseDecl(Decl *DeclNode) override {
     return TraverseNode(
-        DeclNode, DeclNode, [&] { return VisitorBase::TraverseDecl(DeclNode); },
+        DeclNode, DeclNode,
+        [&] { return DynamicRecursiveASTVisitor::TraverseDecl(DeclNode); },
         &Map.PointerParents);
   }
-  bool TraverseTypeLoc(TypeLoc TypeLocNode) {
+  bool TraverseTypeLoc(TypeLoc TypeLocNode) override {
     return TraverseNode(
         TypeLocNode, DynTypedNode::create(TypeLocNode),
-        [&] { return VisitorBase::TraverseTypeLoc(TypeLocNode); },
+        [&] {
+          return DynamicRecursiveASTVisitor::TraverseTypeLoc(TypeLocNode);
+        },
         &Map.OtherParents);
   }
-  bool TraverseNestedNameSpecifierLoc(NestedNameSpecifierLoc NNSLocNode) {
+  bool
+  TraverseNestedNameSpecifierLoc(NestedNameSpecifierLoc NNSLocNode) override {
     return TraverseNode(
         NNSLocNode, DynTypedNode::create(NNSLocNode),
-        [&] { return VisitorBase::TraverseNestedNameSpecifierLoc(NNSLocNode); },
+        [&] {
+          return DynamicRecursiveASTVisitor::TraverseNestedNameSpecifierLoc(
+              NNSLocNode);
+        },
         &Map.OtherParents);
   }
-  bool TraverseAttr(Attr *AttrNode) {
+  bool TraverseAttr(Attr *AttrNode) override {
     return TraverseNode(
-        AttrNode, AttrNode, [&] { return VisitorBase::TraverseAttr(AttrNode); },
+        AttrNode, AttrNode,
+        [&] { return DynamicRecursiveASTVisitor::TraverseAttr(AttrNode); },
         &Map.PointerParents);
   }
-  bool TraverseObjCProtocolLoc(ObjCProtocolLoc ProtocolLocNode) {
+  bool TraverseObjCProtocolLoc(ObjCProtocolLoc ProtocolLocNode) override {
     return TraverseNode(
         ProtocolLocNode, DynTypedNode::create(ProtocolLocNode),
-        [&] { return VisitorBase::TraverseObjCProtocolLoc(ProtocolLocNode); },
+        [&] {
+          return DynamicRecursiveASTVisitor::TraverseObjCProtocolLoc(
+              ProtocolLocNode);
+        },
         &Map.OtherParents);
   }
 
   // Using generic TraverseNode for Stmt would prevent data-recursion.
-  bool dataTraverseStmtPre(Stmt *StmtNode) {
+  bool dataTraverseStmtPre(Stmt *StmtNode) override {
     addParent(StmtNode, &Map.PointerParents);
     ParentStack.push_back(DynTypedNode::create(*StmtNode));
     return true;
   }
-  bool dataTraverseStmtPost(Stmt *StmtNode) {
+  bool dataTraverseStmtPost(Stmt *StmtNode) override {
     ParentStack.pop_back();
     return true;
   }
diff --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
index 3d01a70395a9bb..d18a7e90c59e36 100644
--- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -19,7 +19,7 @@
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclCXX.h"
-#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/DynamicRecursiveASTVisitor.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringMap.h"
@@ -87,11 +87,8 @@ struct MemoizedMatchResult {
 
 // A RecursiveASTVisitor that traverses all children or all descendants of
 // a node.
-class MatchChildASTVisitor
-    : public RecursiveASTVisitor<MatchChildASTVisitor> {
+class MatchChildASTVisitor : public DynamicRecursiveASTVisitor {
 public:
-  typedef RecursiveASTVisitor<MatchChildASTVisitor> VisitorBase;
-
   // Creates an AST visitor that matches 'matcher' on all children or
   // descendants of a traversed node. max_depth is the maximum depth
   // to traverse: use 1 for matching the children and INT_MAX for
@@ -101,8 +98,10 @@ class MatchChildASTVisitor
                        bool IgnoreImplicitChildren,
                        ASTMatchFinder::BindKind Bind)
       : Matcher(Matcher), Finder(Finder), Builder(Builder), CurrentDepth(0),
-        MaxDepth(MaxDepth), IgnoreImplicitChildren(IgnoreImplicitChildren),
-        Bind(Bind), Matches(false) {}
+        MaxDepth(MaxDepth), Bind(Bind), Matches(false) {
+    ShouldVisitTemplateInstantiations = true;
+    ShouldVisitImplicitCode = !IgnoreImplicitChildren;
+  }
 
   // Returns true if a match is found in the subtree rooted at the
   // given AST node. This is done via a set of mutually recursive
@@ -111,7 +110,8 @@ class MatchChildASTVisitor
   //
   //   - Traverse(node) calls BaseTraverse(node) when it needs
   //     to visit the descendants of node.
-  //   - BaseTraverse(node) then calls (via VisitorBase::Traverse*(node))
+  //   - BaseTraverse(node) then calls (via
+  //   DynamicRecursiveASTVisitor::Traverse*(node))
   //     Traverse*(c) for each child c of 'node'.
   //   - Traverse*(c) in turn calls Traverse(c), completing the
   //     recursion.
@@ -151,7 +151,7 @@ class MatchChildASTVisitor
   // The following are overriding methods from the base visitor class.
   // They are public only to allow CRTP to work. They are *not *part
   // of the public API of this class.
-  bool TraverseDecl(Decl *DeclNode) {
+  bool TraverseDecl(Decl *DeclNode) override {
 
     if (DeclNode && DeclNode->isImplicit() &&
         Finder->isTraversalIgnoringImplicitNodes())
@@ -175,26 +175,22 @@ class MatchChildASTVisitor
     return StmtToTraverse;
   }
 
-  bool TraverseStmt(Stmt *StmtNode, DataRecursionQueue *Queue = nullptr) {
-    // If we need to keep track of the depth, we can't perform data recursion.
-    if (CurrentDepth == 0 || (CurrentDepth <= MaxDepth && MaxDepth < INT_MAX))
-      Queue = nullptr;
-
+  bool TraverseStmt(Stmt *StmtNode) override {
     ScopedIncrement ScopedDepth(&CurrentDepth);
     Stmt *StmtToTraverse = getStmtToTraverse(StmtNode);
     if (!StmtToTraverse)
       return true;
 
-    if (IgnoreImplicitChildren && isa<CXXDefaultArgExpr>(StmtNode))
+    if (!ShouldVisitImplicitCode && isa<CXXDefaultArgExpr>(StmtNode))
       return true;
 
     if (!match(*StmtToTraverse))
       return false;
-    return VisitorBase::TraverseStmt(StmtToTraverse, Queue);
+    return DynamicRecursiveASTVisitor::TraverseStmt(StmtToTraverse);
   }
   // We assume that the QualType and the contained type are on the same
   // hierarchy level. Thus, we try to match either of them.
-  bool TraverseType(QualType TypeNode) {
+  bool TraverseType(QualType TypeNode) override {
     if (TypeNode.isNull())
       return true;
     ScopedIncrement ScopedDepth(&CurrentDepth);
@@ -206,7 +202,7 @@ class MatchChildASTVisitor
   }
   // We assume that the TypeLoc, contained QualType and contained Type all are
   // on the same hierarchy level. Thus, we try to match all of them.
-  bool TraverseTypeLoc(TypeLoc TypeLocNode) {
+  bool TraverseTypeLoc(TypeLoc TypeLocNode) override {
     if (TypeLocNode.isNull())
       return true;
     ScopedIncrement ScopedDepth(&CurrentDepth);
@@ -219,11 +215,11 @@ class MatchChildASTVisitor
     // The TypeLoc is matched inside traverse.
     return traverse(TypeLocNode);
   }
-  bool TraverseNestedNameSpecifier(NestedNameSpecifier *NNS) {
+  bool TraverseNestedNameSpecifier(NestedNameSpecifier *NNS) override {
     ScopedIncrement ScopedDepth(&CurrentDepth);
     return (NNS == nullptr) || traverse(*NNS);
   }
-  bool TraverseNestedNameSpecifierLoc(NestedNameSpecifierLoc NNS) {
+  bool TraverseNestedNameSpecifierLoc(NestedNameSpecifierLoc NNS) override {
     if (!NNS)
       return true;
     ScopedIncrement ScopedDepth(&CurrentDepth);
@@ -231,19 +227,19 @@ class MatchChildASTVisitor
       return false;
     return traverse(NNS);
   }
-  bool TraverseConstructorInitializer(CXXCtorInitializer *CtorInit) {
+  bool TraverseConstructorInitializer(CXXCtorInitializer *CtorInit) override {
     if (!CtorInit)
       return true;
     ScopedIncrement ScopedDepth(&CurrentDepth);
     return traverse(*CtorInit);
   }
-  bool TraverseTemplateArgumentLoc(TemplateArgumentLoc TAL) {
+  bool TraverseTemplateArgumentLoc(const TemplateArgumentLoc &TAL) override {
     ScopedIncrement ScopedDepth(&CurrentDepth);
     return traverse(TAL);
   }
-  bool TraverseCXXForRangeStmt(CXXForRangeStmt *Node) {
+  bool TraverseCXXForRangeStmt(CXXForRangeStmt *Node) override {
     if (!Finder->isTraversalIgnoringImplicitNodes())
-      return VisitorBase::TraverseCXXForRangeStmt(Node);
+      return DynamicRecursiveASTVisitor::TraverseCXXForRangeStmt(Node);
     if (!Node)
       return true;
     ScopedIncrement ScopedDepth(&CurrentDepth);
@@ -253,22 +249,24 @@ class MatchChildASTVisitor
     if (!match(*Node->getLoopVariable()))
       return false;
     if (match(*Node->getRangeInit()))
-      if (!VisitorBase::TraverseStmt(Node->getRangeInit()))
+      if (!DynamicRecursiveASTVisitor::TraverseStmt(Node->getRangeInit()))
         return false;
     if (!match(*Node->getBody()))
       return false;
-    return VisitorBase::TraverseStmt(Node->getBody());
+    return DynamicRecursiveASTVisitor::TraverseStmt(Node->getBody());
   }
-  bool TraverseCXXRewrittenBinaryOperator(CXXRewrittenBinaryOperator *Node) {
+  bool TraverseCXXRewrittenBinaryOperator(
+      CXXRewrittenBinaryOperator *Node) override {
     if (!Finder->isTraversalIgnoringImplicitNodes())
-      return VisitorBase::TraverseCXXRewrittenBinaryOperator(Node);
+      return DynamicRecursiveASTVisitor::TraverseCXXRewrittenBinaryOperator(
+          Node);
     if (!Node)
       return true;
     ScopedIncrement ScopedDepth(&CurrentDepth);
 
     return match(*Node->getLHS()) && match(*Node->getRHS());
   }
-  bool TraverseAttr(Attr *A) {
+  bool TraverseAttr(Attr *A) override {...
[truncated]

cor3ntin · 2024-11-27T06:06:26Z

clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp

@@ -266,7 +265,7 @@ class RenameLocFinder : public RecursiveASTVisitor<RenameLocFinder> {
    return true;
  }

-  bool VisitDeclRefExpr(const DeclRefExpr *Expr) {


Why are we removing const here (and other places)?

So RecursiveASTVisitor passes everything around by non-const pointer, and DynamicRecursiveASTVisitor does the same. But because this doesn’t matter for CRTP, we have some visitors that take only non-const pointers, some that take const pointers, and some that mix both seemingly at random. For virtual functions of course, the type has to match exactly, so I had to remove const in a bunch of places.

This makes sense but it is not amazing.
How hard do you think it would be to introduce something like MutableDynamicRecursiveASTVisitor and preserve constness where possible?

How hard do you think it would be to introduce something like MutableDynamicRecursiveASTVisitor and preserve constness where possible?

I mean, it would basically just involve copy-pasting the entire DRAV interface+implementation and adding const everywhere; personally, I don’t really think it’s worth it because there aren’t that many visitors that were using const in the first place, and the entire RAV implementation also isn’t const to begin with...

@AaronBallman @erichkeane

Urgh, yeah... our const correctness with the recursive AST visitor is... bad. One thing I think we'd be better off doing if we cared to make this change (rather than copy/paste) would be to make RecursiveASTVistior take a template parameter that says whether it is const correct-ish and apply it that way. our parameters would have a lot of add_const_if<DeclRefExpr, IsConst> (where add_const_if is a new template we would need to add, and IsConst is the template parameter I referred to).

One thing I think we'd be better off doing if we cared to make this change (rather than copy/paste) would be to make RecursiveASTVistior take a template parameter that says whether it is const correct-ish and apply it that way.

That still doesn’t work too well for DynamicRecursiveASTVisitor—I guess we could make the template parameter a bool and then explicitly instantiate the only two possible instantiations in DynamicRecursiveASTVisitor.cpp...

tbaederr · 2025-01-07T07:50:26Z

clang/lib/AST/ParentMapContext.cpp

    return TraverseNode(
        TypeLocNode, DynTypedNode::create(TypeLocNode),
-        [&] { return VisitorBase::TraverseTypeLoc(TypeLocNode); },
+        [&] {
+          return DynamicRecursiveASTVisitor::TraverseTypeLoc(TypeLocNode);


Does this make sense? It's not the same, isn't it? The previous code would call back into ParentMapContext, but the new one doesn't(?) This should basically be this->TraverseTypeLoc(), right? Am I missing something?

Er, unless I misread something horribly somewhere, in both cases this is statically calling the base class implementation (VisitorBase was RecursiveASTVisitor<ASTVisitor>, i.e. the base class of this thing).

After some discussion around #116823, it was decided that it would be nice to have a `const` variant of `DynamicRecursiveASTVisitor`, so this pr does exactly that by making the main DRAV implementation a template with a single `bool` template parameter that turns several function parameters from a `T*` or `T&` to a `const T*` or `const T&`. Since that made the implementation of a bunch of DRAV functions quite a bit more verbose, I’ve moved most of them to be stamped out by a macro, which imo makes it easier to understand what’s actually going on there. For functions which already accepted `const` parameters in the original RAV implementation, the parameter is `const` in both versions (e.g. `TraverseTemplateArgument()` always takes a `const TemplateArgument&`); conversely, parameters that are passed by value (e.g. in `TraverseType()`, which takes a `QualType` by value) are *not* `const` in either variant (i.e. the `QualType` argument is always just a `QualType`, never a `const QualType`). As a sanity check, I’ve also migrated some random visitor in the static analyser to the `const` version (and indeed, it ends up simplifying the code around that particular visitor actually). It would make sense to do a pass over all visitors and change all which can be `const` use the `const` version, but that can be done in a follow-up pr. The [performance impact](https://llvm-compile-time-tracker.com/compare.php?from=e3cd88a7be1dfd912bb6e7c7e888e7b442ffb5de&to=d55c5afe4a485b6d0431386e6f45cb44c1fc8883&stat=instructions:u) of this change seems to be negligible. Clang’s binary size went up by 0.5%, but that’s expected considering that this effectively adds an extra instantiation of `RecursiveASTVisitor`. Fortunately, this is of course a one-time cost.

Sirraide added 4 commits November 19, 2024 13:28

[Clang] [NFC] Migrate visitors in AST

2b29d67

Migrate ASTMatcher visitor

0dc0808

Migrate visitors in CodeGen

1f71d14

Migrate yet more visitors

e2fefce

Sirraide added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Nov 19, 2024

Sirraide requested review from cor3ntin and AaronBallman November 19, 2024 15:25

Sirraide requested a review from cyndyishida as a code owner November 19, 2024 15:25

llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. debuginfo labels Nov 19, 2024

Sirraide changed the title ~~[Clang] [NFC] Migrate visitors in AST~~ [Clang] [NFC] Refactor more AST visitors Nov 19, 2024

Merge branch 'main' into perf/migrate-visitors-3

2293a6f

cor3ntin reviewed Nov 27, 2024

View reviewed changes

tbaederr reviewed Jan 7, 2025

View reviewed changes

Sirraide mentioned this pull request Jan 15, 2025

[Clang] [NFC] Introduce ConstDynamicRecursiveASTVisitor #122991

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang] [NFC] Refactor more AST visitors #116823

[Clang] [NFC] Refactor more AST visitors #116823

Uh oh!

Sirraide commented Nov 19, 2024

Uh oh!

llvmbot commented Nov 19, 2024 •

edited

Loading

Uh oh!

cor3ntin Nov 27, 2024

Uh oh!

Sirraide Nov 27, 2024

Uh oh!

cor3ntin Nov 28, 2024

Uh oh!

Sirraide Nov 28, 2024

Uh oh!

cor3ntin Dec 2, 2024

Uh oh!

erichkeane Dec 2, 2024

Uh oh!

Sirraide Dec 2, 2024

Uh oh!

tbaederr Jan 7, 2025

Uh oh!

Sirraide Jan 13, 2025

Uh oh!

Uh oh!

[Clang] [NFC] Refactor more AST visitors #116823

Are you sure you want to change the base?

[Clang] [NFC] Refactor more AST visitors #116823

Uh oh!

Conversation

Sirraide commented Nov 19, 2024

Uh oh!

llvmbot commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented Nov 19, 2024 •

edited

Loading