From: Ilya Biryukov <ibiryukov@google.com>
Date: Tue, 6 Aug 2019 15:46:12 +0000 (+0000)
Subject: [AST] Traverse attributes inside DEF_TRAVERSE_DECL macro
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=888cb09800dc23e1090f968b94184260c7f88308;p=clang

[AST] Traverse attributes inside DEF_TRAVERSE_DECL macro

Summary:
Instead of traversing inside the TraverseDecl() function.
Previously the attributes were traversed after Travese(Some)Decl
returns.

Logically attributes are properties of particular Decls and should be
traversed alongside other "child" nodes.

None of the tests relied on this behavior, hopefully this is an indication
that the change is relatively safe.

This change started with a discussion on cfe-dev, for details see:
https://lists.llvm.org/pipermail/cfe-dev/2019-July/062899.html

Reviewers: rsmith, gribozavr

Reviewed By: gribozavr

Subscribers: mgorny, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D64907

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@368052 91177308-0d34-0410-b5e6-96231b3b80d8
---

diff --git a/include/clang/AST/RecursiveASTVisitor.h b/include/clang/AST/RecursiveASTVisitor.h
index eb047bd5c5..918e5b084d 100644
--- a/include/clang/AST/RecursiveASTVisitor.h
+++ b/include/clang/AST/RecursiveASTVisitor.h
@@ -722,12 +722,6 @@ bool RecursiveASTVisitor<Derived>::TraverseDecl(Decl *D) {
     break;
 #include "clang/AST/DeclNodes.inc"
   }
-
-  // Visit any attributes attached to this declaration.
-  for (auto *I : D->attrs()) {
-    if (!getDerived().TraverseAttr(I))
-      return false;
-  }
   return true;
 }
 
@@ -1407,6 +1401,11 @@ bool RecursiveASTVisitor<Derived>::TraverseDeclContextHelper(DeclContext *DC) {
     { CODE; }                                                                  \
     if (ReturnValue && ShouldVisitChildren)                                    \
       TRY_TO(TraverseDeclContextHelper(dyn_cast<DeclContext>(D)));             \
+    if (ReturnValue) {                                                         \
+      /* Visit any attributes attached to this declaration. */                 \
+      for (auto *I : D->attrs())                                               \
+        TRY_TO(getDerived().TraverseAttr(I));                                  \
+    }                                                                          \
     if (ReturnValue && getDerived().shouldTraversePostOrder())                 \
       TRY_TO(WalkUpFrom##DECL(D));                                             \
     return ReturnValue;                                                        \
diff --git a/unittests/AST/CMakeLists.txt b/unittests/AST/CMakeLists.txt
index 333aded9ad..c4940e8ace 100644
--- a/unittests/AST/CMakeLists.txt
+++ b/unittests/AST/CMakeLists.txt
@@ -26,6 +26,7 @@ add_clang_unittest(ASTTests
   Language.cpp
   NamedDeclPrinterTest.cpp
   OMPStructuredBlockTest.cpp
+  RecursiveASTVisitorTest.cpp
   SourceLocationTest.cpp
   StmtPrinterTest.cpp
   StructuralEquivalenceTest.cpp
diff --git a/unittests/AST/RecursiveASTVisitorTest.cpp b/unittests/AST/RecursiveASTVisitorTest.cpp
new file mode 100644
index 0000000000..658adb0e57
--- /dev/null
+++ b/unittests/AST/RecursiveASTVisitorTest.cpp
@@ -0,0 +1,106 @@
+//===- unittest/AST/RecursiveASTVisitorTest.cpp ---------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Attr.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/FunctionExtras.h"
+#include "llvm/ADT/STLExtras.h"
+#include "gmock/gmock-generated-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include <cassert>
+
+using namespace clang;
+using ::testing::ElementsAre;
+
+namespace {
+class ProcessASTAction : public clang::ASTFrontendAction {
+public:
+  ProcessASTAction(llvm::unique_function<void(clang::ASTContext &)> Process)
+      : Process(std::move(Process)) {
+    assert(this->Process);
+  }
+
+  std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
+                                                 StringRef InFile) {
+    class Consumer : public ASTConsumer {
+    public:
+      Consumer(llvm::function_ref<void(ASTContext &CTx)> Process)
+          : Process(Process) {}
+
+      void HandleTranslationUnit(ASTContext &Ctx) override { Process(Ctx); }
+
+    private:
+      llvm::function_ref<void(ASTContext &CTx)> Process;
+    };
+
+    return llvm::make_unique<Consumer>(Process);
+  }
+
+private:
+  llvm::unique_function<void(clang::ASTContext &)> Process;
+};
+
+enum class VisitEvent {
+  StartTraverseFunction,
+  EndTraverseFunction,
+  StartTraverseAttr,
+  EndTraverseAttr
+};
+
+class CollectInterestingEvents
+    : public RecursiveASTVisitor<CollectInterestingEvents> {
+public:
+  bool TraverseFunctionDecl(FunctionDecl *D) {
+    Events.push_back(VisitEvent::StartTraverseFunction);
+    bool Ret = RecursiveASTVisitor::TraverseFunctionDecl(D);
+    Events.push_back(VisitEvent::EndTraverseFunction);
+
+    return Ret;
+  }
+
+  bool TraverseAttr(Attr *A) {
+    Events.push_back(VisitEvent::StartTraverseAttr);
+    bool Ret = RecursiveASTVisitor::TraverseAttr(A);
+    Events.push_back(VisitEvent::EndTraverseAttr);
+
+    return Ret;
+  }
+
+  std::vector<VisitEvent> takeEvents() && { return std::move(Events); }
+
+private:
+  std::vector<VisitEvent> Events;
+};
+
+std::vector<VisitEvent> collectEvents(llvm::StringRef Code) {
+  CollectInterestingEvents Visitor;
+  clang::tooling::runToolOnCode(
+      new ProcessASTAction(
+          [&](clang::ASTContext &Ctx) { Visitor.TraverseAST(Ctx); }),
+      Code);
+  return std::move(Visitor).takeEvents();
+}
+} // namespace
+
+TEST(RecursiveASTVisitorTest, AttributesInsideDecls) {
+  /// Check attributes are traversed inside TraverseFunctionDecl.
+  llvm::StringRef Code = R"cpp(
+__attribute__((annotate("something"))) int foo() { return 10; }
+  )cpp";
+
+  EXPECT_THAT(collectEvents(Code),
+              ElementsAre(VisitEvent::StartTraverseFunction,
+                          VisitEvent::StartTraverseAttr,
+                          VisitEvent::EndTraverseAttr,
+                          VisitEvent::EndTraverseFunction));
+}