PATCH: update RecursiveASTVisitor to visit attributes.
Aaron Ballman
aaron at aaronballman.com
Wed Dec 18 12:44:07 PST 2013
On Wed, Dec 18, 2013 at 2:36 PM, Delesley Hutchins <delesley at google.com> wrote:
> This patch adds Visit*Attr(A) methods to RecursiveASTVisitor, and
> updates tablegen to emit the appropriate traversal routines. Right
> now, the main use case is thread safety attributes. These attributes
> contain nested expressions that are currently not being visited, which
> means that they are invisible to various tools that use the visitor to
> collect expressions.
Nits and questions below.
> Question: Do we have test code for RecursiveASTVisitor? Where?
RecursiveASTVisitorTest.cpp looks promising, in the Tooling project.
> diff --git a/include/clang/AST/CMakeLists.txt b/include/clang/AST/CMakeLists.txt
> index ba54fa2..260734f 100644
> --- a/include/clang/AST/CMakeLists.txt
> +++ b/include/clang/AST/CMakeLists.txt
> @@ -13,6 +13,11 @@ clang_tablegen(AttrDump.inc -gen-clang-attr-dump
> SOURCE ../Basic/Attr.td
> TARGET ClangAttrDump)
>
> +clang_tablegen(AttrVisitor.inc -gen-clang-attr-ast-visitor
> + -I ${CMAKE_CURRENT_SOURCE_DIR}/../../
> + SOURCE ../Basic/Attr.td
> + TARGET ClangAttrVisitor)
> +
> clang_tablegen(StmtNodes.inc -gen-clang-stmt-nodes
> SOURCE ../Basic/StmtNodes.td
> TARGET ClangStmtNodes)
> diff --git a/include/clang/AST/Makefile b/include/clang/AST/Makefile
> index ae84bcf..85e6449 100644
> --- a/include/clang/AST/Makefile
> +++ b/include/clang/AST/Makefile
> @@ -1,6 +1,6 @@
> CLANG_LEVEL := ../../..
> TD_SRC_DIR = $(PROJ_SRC_DIR)/../Basic
> -BUILT_SOURCES = Attrs.inc AttrImpl.inc AttrDump.inc \
> +BUILT_SOURCES = Attrs.inc AttrImpl.inc AttrDump.inc AttrVisitor.inc \
> StmtNodes.inc DeclNodes.inc \
> CommentNodes.inc CommentHTMLTags.inc \
> CommentHTMLTagsProperties.inc \
> @@ -30,6 +30,12 @@ $(ObjDir)/AttrDump.inc.tmp : $(TD_SRC_DIR)/Attr.td $(CLANG_TBLGEN) \
> $(Verb) $(ClangTableGen) -gen-clang-attr-dump -o $(call SYSPATH, $@) \
> -I $(PROJ_SRC_DIR)/../../ $<
>
> +$(ObjDir)/AttrVisitor.inc.tmp : $(TD_SRC_DIR)/Attr.td $(CLANG_TBLGEN) \
> + $(ObjDir)/.dir
> + $(Echo) "Building Clang attribute AST visitor with tblgen"
> + $(Verb) $(ClangTableGen) -gen-clang-attr-ast-visitor -o $(call SYSPATH, $@) \
> + -I $(PROJ_SRC_DIR)/../../ $<
> +
> $(ObjDir)/StmtNodes.inc.tmp : $(TD_SRC_DIR)/StmtNodes.td $(CLANG_TBLGEN) \
> $(ObjDir)/.dir
> $(Echo) "Building Clang statement node tables with tblgen"
> diff --git a/lib/AST/CMakeLists.txt b/lib/AST/CMakeLists.txt
> index 461e8b3..e0b3c33 100644
> --- a/lib/AST/CMakeLists.txt
> +++ b/lib/AST/CMakeLists.txt
> @@ -65,6 +65,7 @@ add_dependencies(clangAST
> ClangAttrList
> ClangAttrImpl
> ClangAttrDump
> + ClangAttrVisitor
> ClangCommentCommandInfo
> ClangCommentCommandList
> ClangCommentNodes
> diff --git a/tools/libclang/RecursiveASTVisitor.h b/tools/libclang/RecursiveASTVisitor.h
> index fa574f6..e0bcaa2 100644
> --- a/tools/libclang/RecursiveASTVisitor.h
> +++ b/tools/libclang/RecursiveASTVisitor.h
> @@ -14,6 +14,7 @@
> #ifndef LLVM_CLANG_LIBCLANG_RECURSIVEASTVISITOR_H
> #define LLVM_CLANG_LIBCLANG_RECURSIVEASTVISITOR_H
>
> +#include "clang/AST/Attr.h"
> #include "clang/AST/Decl.h"
> #include "clang/AST/DeclCXX.h"
> #include "clang/AST/DeclFriend.h"
> @@ -175,6 +176,13 @@ public:
> /// otherwise (including when the argument is a Null type location).
> bool TraverseTypeLoc(TypeLoc TL);
>
> + /// \brief 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).
> + bool TraverseAttr(Attr *At);
Would it make sense for this to take a const pointer? Or perhaps even
const reference?
> +
> /// \brief Recursively visit a declaration, by dispatching to
> /// Traverse*Decl() based on the argument's dynamic type.
> ///
> @@ -241,6 +249,14 @@ public:
>
> // ---- Methods on Stmts ----
>
> + // \brief Visit an attribute.
> + bool VisitAttr(Attr *A) { return true; }
> +
> + // Declare Traverse* and empty Visit* for all Attr classes.
> +#define ATTR_VISITOR_DECLS_ONLY
> +#include "clang/AST/AttrVisitor.inc"
> +#undef ATTR_VISITOR_DECLS_ONLY
> +
> // Declare Traverse*() for all concrete Stmt classes.
> #define ABSTRACT_STMT(STMT)
> #define STMT(CLASS, PARENT) \
> @@ -553,6 +569,10 @@ bool RecursiveASTVisitor<Derived>::TraverseTypeLoc(TypeLoc TL) {
> }
>
>
> +// Define the Traverse*Attr(Attr* A) methods
> +#include "clang/AST/AttrVisitor.inc"
> +
> +
> template<typename Derived>
> bool RecursiveASTVisitor<Derived>::TraverseDecl(Decl *D) {
> if (!D)
> @@ -571,6 +591,12 @@ bool RecursiveASTVisitor<Derived>::TraverseDecl(Decl *D) {
> #include "clang/AST/DeclNodes.inc"
> }
>
> + // Visit any attributes attached to this declaration.
> + AttrVec &ArgAttrs = D->getAttrs();
> + for(unsigned i = 0; i < ArgAttrs.size(); ++i) {
> + if (!TraverseAttr(ArgAttrs[i]))
> + return false;
> + }
This would probably be better served by using an attr_iterator via
attr_begin/attr_end; it would then fix an assert crash which will
happen if the Decl has no attributes attached to it.
> return true;
> }
>
> diff --git a/utils/TableGen/ClangAttrEmitter.cpp b/utils/TableGen/ClangAttrEmitter.cpp
> index 3b2b9ce..7fbe2d3 100644
> --- a/utils/TableGen/ClangAttrEmitter.cpp
> +++ b/utils/TableGen/ClangAttrEmitter.cpp
> @@ -152,6 +152,7 @@ namespace {
> // These functions print the argument contents formatted in different ways.
> virtual void writeAccessors(raw_ostream &OS) const = 0;
> virtual void writeAccessorDefinitions(raw_ostream &OS) const {}
> + virtual void writeASTVisitorTraversal(raw_ostream &OS) const {}
> virtual void writeCloneArgs(raw_ostream &OS) const = 0;
> virtual void writeTemplateInstantiationArgs(raw_ostream &OS) const = 0;
> virtual void writeTemplateInstantiation(raw_ostream &OS) const {}
> @@ -802,6 +803,10 @@ namespace {
> : SimpleArgument(Arg, Attr, "Expr *")
> {}
>
> + virtual void writeASTVisitorTraversal(raw_ostream &OS) const {
> + OS << " getDerived().TraverseStmt(A->get" << getUpperName() << "());\n";
> + }
> +
> void writeTemplateInstantiationArgs(raw_ostream &OS) const {
> OS << "tempInst" << getUpperName();
> }
> @@ -834,6 +839,18 @@ namespace {
> : VariadicArgument(Arg, Attr, "Expr *")
> {}
>
> + virtual void writeASTVisitorTraversal(raw_ostream &OS) const {
> + OS << " {\n";
> + OS << " " << getType() << " *I = A->" << getLowerName()
> + << "_begin();\n";
> + OS << " " << getType() << " *E = A->" << getLowerName()
> + << "_end();\n";
> + OS << " for (; I != E; ++I) {\n";
> + OS << " getDerived().TraverseStmt(*I);\n";
> + OS << " }\n";
> + OS << " }\n";
> + }
> +
> void writeTemplateInstantiationArgs(raw_ostream &OS) const {
> OS << "tempInst" << getUpperName() << ", "
> << "A->" << getLowerName() << "_size()";
> @@ -1568,6 +1585,95 @@ void EmitClangAttrSpellingListIndex(RecordKeeper &Records, raw_ostream &OS) {
> OS << " return 0;\n";
> }
>
> +// Emits code used by RecursiveASTVisitor to visit attributes
> +void EmitClangAttrASTVisitor(RecordKeeper &Records, raw_ostream &OS) {
> + emitSourceFileHeader("Used by RecursiveASTVisitor to visit attributes.", OS);
> +
> + std::vector<Record*> Attrs = Records.getAllDerivedDefinitions("Attr");
> +
> + // Write method declarations for Traverse* methods.
> + // We do here because we only generate methods for attributes that
> + // are declared as ASTNodes.
Small typo (we do here because)
> + OS << "#ifdef ATTR_VISITOR_DECLS_ONLY\n\n";
> + for (std::vector<Record*>::iterator I = Attrs.begin(), E = Attrs.end();
> + I != E; ++I) {
> + Record &R = **I;
> + if (!R.getValueAsBit("ASTNode"))
> + continue;
Do we want to filter out any statement or type attributes?
> + OS << " bool Traverse"
> + << R.getName() << "Attr(" << R.getName() << "Attr *A);\n";
> + OS << " bool Visit"
> + << R.getName() << "Attr(" << R.getName() << "Attr *A) {\n"
> + << " return true; \n"
> + << " };\n";
> + }
> + OS << "\n#else // ATTR_VISITOR_DECLS_ONLY\n\n";
> +
> + // Write individual Traverse* methods for each attribute class.
> + for (std::vector<Record*>::iterator I = Attrs.begin(), E = Attrs.end();
> + I != E; ++I) {
> + Record &R = **I;
> + if (!R.getValueAsBit("ASTNode"))
> + continue;
Same filtering question here.
> +
> + OS << "template <typename Derived>\n"
> + << "bool RecursiveASTVisitor<Derived>::Traverse"
> + << R.getName() << "Attr(" << R.getName() << "Attr *A) {\n"
> + << " if (!getDerived().VisitAttr(A))\n"
> + << " return false;\n"
> + << " if (!getDerived().Visit" << R.getName() << "Attr(A))\n"
> + << " return false;\n";
> +
> + std::vector<Record*> ArgRecords = R.getValueAsListOfDefs("Args");
> + std::vector<Argument*> Args;
> + std::vector<Argument*>::iterator ai, ae;
> + Args.reserve(ArgRecords.size());
> +
> + for (std::vector<Record*>::iterator ri = ArgRecords.begin(),
> + re = ArgRecords.end();
> + ri != re; ++ri) {
> + Record &ArgRecord = **ri;
> + Argument *Arg = createArgument(ArgRecord, R.getName());
> + assert(Arg);
> + Args.push_back(Arg);
> + }
> + ae = Args.end();
> +
> + for (ai = Args.begin(); ai != ae; ++ai) {
> + (*ai)->writeASTVisitorTraversal(OS);
> + }
Spurious curly braces; but I'm wondering whether this for loop is even
required. Couldn't you accomplish the same thing without the Args
vector by just calling writeASTVisitorTraversal after creating the
Argument object?
On a side note, I just realized we're leaking those Argument objects
all over the place. Oops.
> +
> + OS << " return true;\n";
> + OS << "}\n\n";
> + }
> +
> +
Extra newline.
> + // Write generic Traverse routine
> + OS << "template <typename Derived>\n"
> + << "bool RecursiveASTVisitor<Derived>::TraverseAttr(Attr *A) {\n"
> + << " if (!A)\n"
> + << " return true;\n"
> + << "\n"
> + << " switch (A->getKind()) {\n"
> + << " default:\n"
> + << " return true;\n";
> +
> + for (std::vector<Record*>::iterator I = Attrs.begin(), E = Attrs.end();
> + I != E; ++I) {
> + Record &R = **I;
> + if (!R.getValueAsBit("ASTNode"))
> + continue;
Filtering question here as well.
> +
> + OS << " case attr::" << R.getName() << ":\n"
> + << " return getDerived().Traverse" << R.getName() << "Attr("
> + << "cast<" << R.getName() << "Attr>(A));\n";
> + }
> + OS << " }\n"; // end case
> + OS << "}\n"; // end function
> + OS << "#endif // ATTR_VISITOR_DECLS_ONLY\n";
> +}
> +
> +
> // Emits the LateParsed property for attributes.
> void EmitClangAttrLateParsedList(RecordKeeper &Records, raw_ostream &OS) {
> emitSourceFileHeader("llvm::StringSwitch code to match late parsed "
> diff --git a/utils/TableGen/TableGen.cpp b/utils/TableGen/TableGen.cpp
> index 0e45d81..e0cd027 100644
> --- a/utils/TableGen/TableGen.cpp
> +++ b/utils/TableGen/TableGen.cpp
> @@ -32,6 +32,7 @@ enum ActionType {
> GenClangAttrPCHWrite,
> GenClangAttrSpellingList,
> GenClangAttrSpellingListIndex,
> + GenClangAttrASTVisitor,
> GenClangAttrLateParsedList,
> GenClangAttrTemplateInstantiate,
> GenClangAttrParsedAttrList,
> @@ -82,6 +83,9 @@ cl::opt<ActionType> Action(
> clEnumValN(GenClangAttrSpellingListIndex,
> "gen-clang-attr-spelling-index",
> "Generate a clang attribute spelling index"),
> + clEnumValN(GenClangAttrASTVisitor,
> + "gen-clang-attr-ast-visitor",
> + "Generate a recursive AST visitor for clang attributes"),
> clEnumValN(GenClangAttrLateParsedList,
> "gen-clang-attr-late-parsed-list",
> "Generate a clang attribute LateParsed list"),
> @@ -171,6 +175,9 @@ bool ClangTableGenMain(raw_ostream &OS, RecordKeeper &Records) {
> case GenClangAttrSpellingListIndex:
> EmitClangAttrSpellingListIndex(Records, OS);
> break;
> + case GenClangAttrASTVisitor:
> + EmitClangAttrASTVisitor(Records, OS);
> + break;
> case GenClangAttrLateParsedList:
> EmitClangAttrLateParsedList(Records, OS);
> break;
> diff --git a/utils/TableGen/TableGenBackends.h b/utils/TableGen/TableGenBackends.h
> index 8904287..d9256d8 100644
> --- a/utils/TableGen/TableGenBackends.h
> +++ b/utils/TableGen/TableGenBackends.h
> @@ -38,6 +38,7 @@ void EmitClangAttrPCHRead(RecordKeeper &Records, raw_ostream &OS);
> void EmitClangAttrPCHWrite(RecordKeeper &Records, raw_ostream &OS);
> void EmitClangAttrSpellingList(RecordKeeper &Records, raw_ostream &OS);
> void EmitClangAttrSpellingListIndex(RecordKeeper &Records, raw_ostream &OS);
> +void EmitClangAttrASTVisitor(RecordKeeper &Records, raw_ostream &OS);
> void EmitClangAttrLateParsedList(RecordKeeper &Records, raw_ostream &OS);
> void EmitClangAttrTemplateInstantiate(RecordKeeper &Records, raw_ostream &OS);
> void EmitClangAttrParsedAttrList(RecordKeeper &Records, raw_ostream &OS);
>
~Aaron
More information about the llvm-commits
mailing list