[Clang] [NFC] Introduce DynamicRecursiveASTVisitor
#110040
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sirraide
added
the
clang:frontend
|
@llvm/pr-subscribers-clang Author: None (Sirraide) ChangesFollowing the discussion on #105195, we decided to split this into multiple prs. This one is just the DRAV implementation. Tests for it will be added in a separate pr because testing this involves just rewriting all (or rather, most) of the RAV tests to use DRAV instead—which has the effect of testing both implementations because DRAV uses RAV internally. I’ve already done that and confirmed that it actually works like it’s supposed to (you can build and run the branch mentioned above if you’re interested). Additional follow-up prs will the subsequently migrate every visitor that can be migrated (most of that work is already done) to use DRAV instead of RAV and also update the documentation to recommend using DRAV if possible. Some way to tell people about the fact that this is a thing would also help—maybe I should make a Discourse post once this gets merged or something like that? Ideally, we’d want people to start using DRAV instead of RAV whenever possible since that’s kind of the whole reason why we’re adding this... There is a big comment in DynamicRecursiveASTVisitor.cpp that explains how it works and why it’s implemented this way; this is the best I managed to come up with—if someone has a simpler solution, I’d be more than happy to pivot to that instead, but I couldn’t think of one. Patch is 31.32 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/110040.diff 3 Files Affected:
diff --git a/clang/include/clang/AST/DynamicRecursiveASTVisitor.h b/clang/include/clang/AST/DynamicRecursiveASTVisitor.h
new file mode 100644
index 00000000000000..f0544df93f705f
--- /dev/null
+++ b/clang/include/clang/AST/DynamicRecursiveASTVisitor.h
@@ -0,0 +1,268 @@
+//===--- DynamicRecursiveASTVisitor.h - Virtual AST Visitor -----*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines the DynamicRecursiveASTVisitor interface, which acts
+// identically to RecursiveASTVisitor, except that it uses virtual dispatch
+// instead of CRTP, which greatly improves compile times and binary size.
+//
+// However, it also comes with limitations in that some of the more seldom
+// utilised features of RecursiveASTVisitor are not supported.
+//
+// Prefer to use this over RecursiveASTVisitor whenever possible.
+//
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_CLANG_AST_DYNAMIC_RECURSIVE_AST_VISITOR_H
+#define LLVM_CLANG_AST_DYNAMIC_RECURSIVE_AST_VISITOR_H
+
+#include "clang/AST/Attr.h"
+#include "clang/AST/ExprConcepts.h"
+#include "clang/AST/TypeLoc.h"
+
+namespace clang {
+class ASTContext;
+
+/// Recursive AST visitor that supports extension via dynamic dispatch.
+///
+/// This only supports some of the more common visitation operations; in
+/// particular, it does not support overriding WalkUpFromX or post-order
+/// traversal.
+///
+/// Instead of functions (e.g. shouldVisitImplicitCode()), this class
+/// uses member variables (e.g. ShouldVisitImplicitCode) to control
+/// visitation behaviour.
+///
+/// RAV features that are NOT supported:
+///
+/// - Visiting attributes,
+/// - Post-order traversal,
+/// - Overriding WalkUpFromX,
+/// - Overriding getStmtChildren().
+///
+/// \see RecursiveASTVisitor.
+class DynamicRecursiveASTVisitor {
+public:
+ /// Whether this visitor should recurse into template instantiations.
+ bool ShouldVisitTemplateInstantiations = false;
+
+ /// Whether this visitor should recurse into the types of TypeLocs.
+ bool ShouldWalkTypesOfTypeLocs = true;
+
+ /// Whether this visitor should recurse into implicit code, e.g.
+ /// implicit constructors and destructors.
+ bool ShouldVisitImplicitCode = false;
+
+ /// Whether this visitor should recurse into lambda body.
+ bool ShouldVisitLambdaBody = true;
+
+protected:
+ DynamicRecursiveASTVisitor() = default;
+ DynamicRecursiveASTVisitor(DynamicRecursiveASTVisitor &&) = default;
+ DynamicRecursiveASTVisitor(const DynamicRecursiveASTVisitor &) = default;
+ DynamicRecursiveASTVisitor &
+ operator=(DynamicRecursiveASTVisitor &&) = default;
+ DynamicRecursiveASTVisitor &
+ operator=(const DynamicRecursiveASTVisitor &) = default;
+
+public:
+ virtual void anchor();
+ virtual ~DynamicRecursiveASTVisitor() = default;
+
+ /// Recursively visits an entire AST, starting from the TranslationUnitDecl.
+ /// \returns false if visitation was terminated early.
+ virtual bool TraverseAST(ASTContext &AST);
+
+ /// Recursively visit an attribute, by dispatching to
+ /// Traverse*Attr() based on the argument's dynamic type.
+ ///
+ /// \returns false if the visitation was terminated early, true
+ /// otherwise (including when the argument is a Null type location).
+ virtual bool TraverseAttr(Attr *At);
+
+ /// Recursively visit a constructor initializer. This
+ /// automatically dispatches to another visitor for the initializer
+ /// expression, but not for the name of the initializer, so may
+ /// be overridden for clients that need access to the name.
+ ///
+ /// \returns false if the visitation was terminated early, true otherwise.
+ virtual bool TraverseConstructorInitializer(CXXCtorInitializer *Init);
+
+ /// Recursively visit a base specifier. This can be overridden by a
+ /// subclass.
+ ///
+ /// \returns false if the visitation was terminated early, true otherwise.
+ virtual bool TraverseCXXBaseSpecifier(const CXXBaseSpecifier &Base);
+
+ /// Recursively visit a declaration, by dispatching to
+ /// Traverse*Decl() based on the argument's dynamic type.
+ ///
+ /// \returns false if the visitation was terminated early, true
+ /// otherwise (including when the argument is NULL).
+ virtual bool TraverseDecl(Decl *D);
+
+ /// Recursively visit a name with its location information.
+ ///
+ /// \returns false if the visitation was terminated early, true otherwise.
+ virtual bool TraverseDeclarationNameInfo(DeclarationNameInfo NameInfo);
+
+ /// Recursively visit a lambda capture. \c Init is the expression that
+ /// will be used to initialize the capture.
+ ///
+ /// \returns false if the visitation was terminated early, true otherwise.
+ virtual bool TraverseLambdaCapture(LambdaExpr *LE, const LambdaCapture *C,
+ Expr *Init);
+
+ /// Recursively visit a C++ nested-name-specifier.
+ ///
+ /// \returns false if the visitation was terminated early, true otherwise.
+ virtual bool TraverseNestedNameSpecifier(NestedNameSpecifier *NNS);
+
+ /// Recursively visit a C++ nested-name-specifier with location
+ /// information.
+ ///
+ /// \returns false if the visitation was terminated early, true otherwise.
+ virtual bool TraverseNestedNameSpecifierLoc(NestedNameSpecifierLoc NNS);
+
+ /// Recursively visit a statement or expression, by
+ /// dispatching to Traverse*() based on the argument's dynamic type.
+ ///
+ /// \returns false if the visitation was terminated early, true
+ /// otherwise (including when the argument is nullptr).
+ virtual bool TraverseStmt(Stmt *S);
+
+ /// Recursively visit a template argument and dispatch to the
+ /// appropriate method for the argument type.
+ ///
+ /// \returns false if the visitation was terminated early, true otherwise.
+ // FIXME: migrate callers to TemplateArgumentLoc instead.
+ virtual bool TraverseTemplateArgument(const TemplateArgument &Arg);
+
+ /// Recursively visit a template argument location and dispatch to the
+ /// appropriate method for the argument type.
+ ///
+ /// \returns false if the visitation was terminated early, true otherwise.
+ virtual bool TraverseTemplateArgumentLoc(const TemplateArgumentLoc &ArgLoc);
+
+ /// Recursively visit a set of template arguments.
+ ///
+ /// \returns false if the visitation was terminated early, true otherwise.
+ // FIXME: take a TemplateArgumentLoc* (or TemplateArgumentListInfo) instead.
+ // Not virtual for now because no-one overrides it.
+ bool TraverseTemplateArguments(ArrayRef<TemplateArgument> Args);
+
+ /// Recursively visit a template name and dispatch to the
+ /// appropriate method.
+ ///
+ /// \returns false if the visitation was terminated early, true otherwise.
+ virtual bool TraverseTemplateName(TemplateName Template);
+
+ /// Recursively visit a type, by dispatching to
+ /// Traverse*Type() based on the argument's getTypeClass() property.
+ ///
+ /// \returns false if the visitation was terminated early, true
+ /// otherwise (including when the argument is a Null type).
+ virtual bool TraverseType(QualType T);
+
+ /// Recursively visit a type with location, by dispatching to
+ /// Traverse*TypeLoc() based on the argument type's getTypeClass() property.
+ ///
+ /// \returns false if the visitation was terminated early, true
+ /// otherwise (including when the argument is a Null type location).
+ virtual bool TraverseTypeLoc(TypeLoc TL);
+
+ /// Recursively visit an Objective-C protocol reference with location
+ /// information.
+ ///
+ /// \returns false if the visitation was terminated early, true otherwise.
+ virtual bool TraverseObjCProtocolLoc(ObjCProtocolLoc ProtocolLoc);
+
+ /// Traverse a concept (requirement).
+ virtual bool TraverseTypeConstraint(const TypeConstraint *C);
+ virtual bool TraverseConceptRequirement(concepts::Requirement *R);
+ virtual bool TraverseConceptTypeRequirement(concepts::TypeRequirement *R);
+ virtual bool TraverseConceptExprRequirement(concepts::ExprRequirement *R);
+ virtual bool TraverseConceptNestedRequirement(concepts::NestedRequirement *R);
+ virtual bool TraverseConceptReference(ConceptReference *CR);
+ virtual bool VisitConceptReference(ConceptReference *CR) { return true; }
+
+ /// Visit a node.
+ virtual bool VisitAttr(Attr *A) { return true; }
+ virtual bool VisitDecl(Decl *D) { return true; }
+ virtual bool VisitStmt(Stmt *S) { return true; }
+ virtual bool VisitType(Type *T) { return true; }
+ virtual bool VisitTypeLoc(TypeLoc TL) { return true; }
+
+ /// Walk up from a node.
+ bool WalkUpFromDecl(Decl *D) { return VisitDecl(D); }
+ bool WalkUpFromStmt(Stmt *S) { return VisitStmt(S); }
+ bool WalkUpFromType(Type *T) { return VisitType(T); }
+ bool WalkUpFromTypeLoc(TypeLoc TL) { return VisitTypeLoc(TL); }
+
+ /// Invoked before visiting a statement or expression via data recursion.
+ ///
+ /// \returns false to skip visiting the node, true otherwise.
+ virtual bool dataTraverseStmtPre(Stmt *S) { return true; }
+
+ /// Invoked after visiting a statement or expression via data recursion.
+ /// This is not invoked if the previously invoked \c dataTraverseStmtPre
+ /// returned false.
+ ///
+ /// \returns false if the visitation was terminated early, true otherwise.
+ virtual bool dataTraverseStmtPost(Stmt *S) { return true; }
+ virtual bool dataTraverseNode(Stmt *S);
+
+#define DEF_TRAVERSE_TMPL_INST(kind) \
+ virtual bool TraverseTemplateInstantiations(kind##TemplateDecl *D);
+ DEF_TRAVERSE_TMPL_INST(Class)
+ DEF_TRAVERSE_TMPL_INST(Var)
+ DEF_TRAVERSE_TMPL_INST(Function)
+#undef DEF_TRAVERSE_TMPL_INST
+
+ // Decls.
+#define ABSTRACT_DECL(DECL)
+#define DECL(CLASS, BASE) virtual bool Traverse##CLASS##Decl(CLASS##Decl *D);
+#include "clang/AST/DeclNodes.inc"
+
+#define DECL(CLASS, BASE) \
+ bool WalkUpFrom##CLASS##Decl(CLASS##Decl *D); \
+ virtual bool Visit##CLASS##Decl(CLASS##Decl *D) { return true; }
+#include "clang/AST/DeclNodes.inc"
+
+ // Stmts.
+#define ABSTRACT_STMT(STMT)
+#define STMT(CLASS, PARENT) virtual bool Traverse##CLASS(CLASS *S);
+#include "clang/AST/StmtNodes.inc"
+
+#define STMT(CLASS, PARENT) \
+ bool WalkUpFrom##CLASS(CLASS *S); \
+ virtual bool Visit##CLASS(CLASS *S) { return true; }
+#include "clang/AST/StmtNodes.inc"
+
+ // Types.
+#define ABSTRACT_TYPE(CLASS, BASE)
+#define TYPE(CLASS, BASE) virtual bool Traverse##CLASS##Type(CLASS##Type *T);
+#include "clang/AST/TypeNodes.inc"
+
+#define TYPE(CLASS, BASE) \
+ bool WalkUpFrom##CLASS##Type(CLASS##Type *T); \
+ virtual bool Visit##CLASS##Type(CLASS##Type *T) { return true; }
+#include "clang/AST/TypeNodes.inc"
+
+ // TypeLocs.
+#define ABSTRACT_TYPELOC(CLASS, BASE)
+#define TYPELOC(CLASS, BASE) \
+ virtual bool Traverse##CLASS##TypeLoc(CLASS##TypeLoc TL);
+#include "clang/AST/TypeLocNodes.def"
+
+#define TYPELOC(CLASS, BASE) \
+ bool WalkUpFrom##CLASS##TypeLoc(CLASS##TypeLoc TL); \
+ virtual bool Visit##CLASS##TypeLoc(CLASS##TypeLoc TL) { return true; }
+#include "clang/AST/TypeLocNodes.def"
+};
+} // namespace clang
+
+#endif // LLVM_CLANG_AST_DYNAMIC_RECURSIVE_AST_VISITOR_H
diff --git a/clang/lib/AST/CMakeLists.txt b/clang/lib/AST/CMakeLists.txt
index 6195a16c2c68db..7e5f68b0efd979 100644
--- a/clang/lib/AST/CMakeLists.txt
+++ b/clang/lib/AST/CMakeLists.txt
@@ -53,6 +53,7 @@ add_clang_library(clangAST
DeclOpenMP.cpp
DeclPrinter.cpp
DeclTemplate.cpp
+ DynamicRecursiveASTVisitor.cpp
ParentMapContext.cpp
Expr.cpp
ExprClassification.cpp
diff --git a/clang/lib/AST/DynamicRecursiveASTVisitor.cpp b/clang/lib/AST/DynamicRecursiveASTVisitor.cpp
new file mode 100644
index 00000000000000..8cfabd9f3e93fe
--- /dev/null
+++ b/clang/lib/AST/DynamicRecursiveASTVisitor.cpp
@@ -0,0 +1,452 @@
+//=== DynamicRecursiveASTVisitor.cpp - Dynamic AST Visitor Implementation -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file implements DynamicRecursiveASTVisitor in terms of the CRTP-based
+// RecursiveASTVisitor.
+//
+//===----------------------------------------------------------------------===//
+#include "clang/AST/DynamicRecursiveASTVisitor.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+
+using namespace clang;
+
+// The implementation of DRAV deserves some explanation:
+//
+// We want to implement DynamicRecursiveASTVisitor without having to inherit or
+// reference RecursiveASTVisitor in any way in the header: if we instantiate
+// RAV in the header, then every user of (or rather every file that uses) DRAV
+// still has to instantiate a RAV, which gets us nowhere. Moreover, even just
+// including RecursiveASTVisitor.h would probably cause some amount of slowdown
+// because we'd have to parse a huge template. For these reasons, the fact that
+// DRAV is implemented using a RAV is solely an implementation detail.
+//
+// As for the implementation itself, DRAV by default acts exactly like a RAV
+// that overrides none of RAV's functions. There are two parts to this:
+//
+// 1. Any function in DRAV has to act like the corresponding function in RAV,
+// unless overridden by a derived class, of course.
+//
+// 2. Any call to a function by the RAV implementation that DRAV allows to be
+// overridden must be transformed to a virtual call on the user-provided
+// DRAV object: if some function in RAV calls e.g. TraverseCallExpr()
+// during traversal, then the derived class's TraverseCallExpr() must be
+// called (provided it overrides TraverseCallExpr()).
+//
+// The 'Impl' class is a helper that connects the two implementations; it is
+// a wrapper around a reference to a DRAV that is itself a RecursiveASTVisitor.
+// It overrides every function in RAV *that is virtual in DRAV* to perform a
+// virtual call on its DRAV reference. This accomplishes point 2 above.
+//
+// Point 1 is accomplished by, first, having the base class implementation of
+// each of the virtual functions construct an Impl object (which is actually
+// just a no-op), passing in itself so that any virtual calls use the right
+// vtable. Secondly, it then calls RAV's implementation of that same function
+// *on Impl* (using a qualified call so that we actually call into the RAV
+// implementation instead of Impl's version of that same function); this way,
+// we both execute RAV's implementation for this function only and ensure that
+// calls to subsequent functions call into Impl via CRTP (and Impl then calls
+// back into DRAV and so on).
+//
+// While this ends up constructing a lot of Impl instances (almost one per
+// function call), this doesn't really matter since Impl just holds a single
+// pointer, and everything in this file should get inlined into all the DRAV
+// functions here anyway.
+//
+//===----------------------------------------------------------------------===//
+//
+// The following illustrates how a call to an (overridden) function is actually
+// resolved: given some class 'Derived' that derives from DRAV and overrides
+// TraverseStmt(), if we are traversing some AST, and TraverseStmt() is called
+// by the RAV implementation, the following happens:
+//
+// 1. Impl::TraverseStmt() overrides RAV::TraverseStmt() via CRTP, so the
+// former is called.
+//
+// 2. Impl::TraverseStmt() performs a virtual call to the visitor (which is
+// an instance to Derived), so Derived::TraverseStmt() is called.
+//
+// End result: Derived::TraverseStmt() is executed.
+//
+// Suppose some other function, e.g. TraverseCallExpr(), which is NOT overridden
+// by Derived is called, we get:
+//
+// 1. Impl::TraverseCallExpr() overrides RAV::TraverseCallExpr() via CRTP,
+// so the former is called.
+//
+// 2. Impl::TraverseCallExpr() performs a virtual call, but since Derived
+// does not override that function, DRAV::TraverseCallExpr() is called.
+//
+// 3. DRAV::TraverseCallExpr() creates a new instance of Impl, passing in
+// itself (this doesn't change that the pointer is an instance of Derived);
+// it then calls RAV::TraverseCallExpr() on the Impl object, which actually
+// ends up executing RAV's implementation because we used a qualified
+// function call.
+//
+// End result: RAV::TraverseCallExpr() is executed,
+namespace {
+struct Impl : RecursiveASTVisitor<Impl> {
+ DynamicRecursiveASTVisitor &Visitor;
+ Impl(DynamicRecursiveASTVisitor &Visitor) : Visitor(Visitor) {}
+
+ bool shouldVisitTemplateInstantiations() const {
+ return Visitor.ShouldVisitTemplateInstantiations;
+ }
+
+ bool shouldWalkTypesOfTypeLocs() const {
+ return Visitor.ShouldWalkTypesOfTypeLocs;
+ }
+
+ bool shouldVisitImplicitCode() const {
+ return Visitor.ShouldVisitImplicitCode;
+ }
+
+ bool shouldVisitLambdaBody() const { return Visitor.ShouldVisitLambdaBody; }
+
+ // Supporting post-order would be very hard because of quirks of the
+ // RAV implementation that only work with CRTP. It also is only used
+ // by less than 5 visitors in the entire code base.
+ bool shouldTraversePostOrder() const { return false; }
+
+ bool TraverseAST(ASTContext &AST) { return Visitor.TraverseAST(AST); }
+ bool TraverseAttr(Attr *At) { return Visitor.TraverseAttr(At); }
+ bool TraverseDecl(Decl *D) { return Visitor.TraverseDecl(D); }
+ bool TraverseType(QualType T) { return Visitor.TraverseType(T); }
+ bool TraverseTypeLoc(TypeLoc TL) { return Visitor.TraverseTypeLoc(TL); }
+ bool TraverseStmt(Stmt *S) { return Visitor.TraverseStmt(S); }
+
+ bool TraverseConstructorInitializer(CXXCtorInitializer *Init) {
+ return Visitor.TraverseConstructorInitializer(Init);
+ }
+
+ bool TraverseTemplateArgument(const TemplateArgument &Arg) {
+ return Visitor.TraverseTemplateArgument(Arg);
+ }
+
+ bool TraverseTemplateArgumentLoc(const TemplateArgumentLoc &ArgLoc) {
+ return Visitor.TraverseTemplateArgumentLoc(ArgLoc);
+ }
+
+ bool TraverseTemplateName(TemplateName Template) {
+ return Visitor.TraverseTemplateName(Template);
+ }
+
+ bool TraverseObjCProtocolLoc(ObjCProtocolLoc ProtocolLoc) {
+ return Visitor.TraverseObjCProtocolLoc(ProtocolLoc);
+ }
+
+ bool TraverseTypeConstraint(const TypeConstraint *C) {
+ return Visitor.TraverseTypeConstraint(C);
+ }
+ bool TraverseConceptRequirement(concepts::Requirement *R) {
+ return Visitor.TraverseConceptRequirement(R);
+ }
+ bool TraverseConceptTypeRequirement(concepts::TypeRequirement *R) {
+ return Visitor.TraverseConceptTypeRequirement(R);
+ }
+ bool TraverseConceptExprRequirement(concepts::ExprRequirement *R) {
+ return Visitor.TraverseConceptExprRequirement(R);
+ }
+ bool TraverseConceptNestedRequirement(concepts::NestedRequirement *R) {
+ return Visitor.TraverseConceptNestedRequirement(R);
+ }
+
+ bool TraverseConceptReference(ConceptReference *CR) {
+ return Visitor.TraverseConceptReference(CR);
+ }
+
+ bool TraverseCXXBaseSpecifier(const CXXBaseSpecifier &Base) {
+ return Visitor.TraverseCXXBaseSpecifier(Base);
+ }
+
+ bool TraverseDeclarationNameInfo(DeclarationNameInfo NameInfo) {
+ return Visitor.TraverseDeclarationNameInfo(NameInfo);
+ }
+
+ bool TraverseLambdaCapture(LambdaExpr *LE, const LambdaCapture *C,
+ Expr *Init) {
+ return Visitor.TraverseLambdaCapture(LE, C, Init);
+ }
+
+ bool TraverseNestedNameSpecifier(NestedNameSpecifier *NNS) {
+ return Visitor.TraverseNestedNameSpecifier(NNS);
+ }
+
+ bool TraverseNestedNameSpecifierLoc(NestedNameSpecifierLoc NNS) {
+ return Visitor.TraverseNestedNameSpecifierLoc(NNS);
+ }
+
+ b...
[truncated]
|
Sirraide
commented
Sep 25, 2024
Sirraide
commented
Sep 25, 2024
|
ping |
|
ping |
|
@AaronBallman ping |
cor3ntin
reviewed
Oct 22, 2024
AaronBallman
reviewed
Nov 4, 2024
For this pr? None, because the DRAV is still unused here. But I can post the compile-time tracker link for the old branch that has changes to a bunch of visitors, and which should be an indicator as to what effect this is going to have in the long run once all/most visitors have been migrated. |
|
AaronBallman
approved these changes
Nov 5, 2024
There was a problem hiding this comment.
I think these are a net positive change; @nikic are you comfortable with the performance numbers? I feel like the changes come awfully close to falling out in the noise aside from the positive improvements with building Clang, but you look at compile time performance numbers more than I do, so you may have a better sense for whether the changes are acceptable.
Signing off with a LGTM, but please wait to land until @nikic has weighed in.
Based on my previous analysis, the compilation time impact here is not actually because the visitor becomes appreciably slower, but because we have more dynamic relocations on startup due to dynamic relocations for the large vtables of these visitors. So only tiny compilations should see any significant impact. Now, tiny compilations aren't entirely irrelevant (this is the bottleneck for things like configure / cmake tests), but I'm also not overly concerned about it, and I think the benefits here substantially outweigh the costs. |
|
Thanks for the reviews! The next step for me will be to start migrating the AST visitor tests (or rather, take all the changes that I have on the other branch and make a new pr). |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/55/builds/3351 Here is the relevant piece of the build log for the reference |
|
CI failure seems unrlated considering that the DRAV is quite literally unused at the moment |
Sirraide
added a commit
that referenced
this pull request
Nov 15, 2024
…o use DynamicRecursiveASTVisitor (#115144) This pr refactors all recursive AST visitors in `Sema`, `Analyze`, and `StaticAnalysis` to inherit from DRAV instead. This is over half of the visitors that inherit from RAV directly. See also #115132, #110040, #93462 LLVM Compile-Time Tracker link for this branch: https://llvm-compile-time-tracker.com/compare.php?from=5adb5c05a2e9f31385fbba8b0436cbc07d91a44d&to=b58e589a86c06ba28d4d90613864d10be29aa5ba&stat=instructions%3Au
This was referenced Nov 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Following the discussion on #105195, we decided to split this into multiple prs. This one is just the DRAV implementation.
Tests for it will be added in a separate pr because testing this involves just rewriting all (or rather, most) of the RAV tests to use DRAV instead—which has the effect of testing both implementations because DRAV uses RAV internally. I’ve already done that and confirmed that it actually works like it’s supposed to (you can build and run the branch mentioned above if you’re interested).
Additional follow-up prs will the subsequently migrate every visitor that can be migrated (most of that work is already done) to use DRAV instead of RAV and also update the documentation to recommend using DRAV if possible.
Some way to tell people about the fact that this is a thing would also help—maybe I should make a Discourse post once this gets merged or something like that? Ideally, we’d want people to start using DRAV instead of RAV whenever possible since that’s kind of the whole reason why we’re adding this...
There is a big comment in DynamicRecursiveASTVisitor.cpp that explains how it works and why it’s implemented this way; this is the best I managed to come up with—if someone has a simpler solution, I’d be more than happy to pivot to that instead, but I couldn’t think of one.