[AST] Traverse attributes inside DEF_TRAVERSE_DECL macro

[ilya-biryukov]

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

[ilya-biryukov]

• Visit attributes before visiting the Decl

[ilya-biryukov]

Wanted to write unit tests, but couldn't find any that check traversal order. I'm aware of Tooling/RecursiveASTVisitorTests, but they mostly check that implicit nodes are visited.
Do we have other tests for traversals?

[gribozavr]

Looks like a good idea to me.

Regarding tests, I couldn't find existing tests that check order either. Seems like you'd need to make some minimal infrastructure for that.

[ilya-biryukov]

• Add a test

[ilya-biryukov]

• Reformat

[gribozavr]

clang/unittests/AST/RecursiveASTVisitorTest.cpp

107 ↗

(On Diff #212592)

Please add a newline.

[ilya-biryukov]

• Add the missing newline

clang/include/clang/AST/RecursiveASTVisitor.h

 #define ABSTRACT_DECL(DECL)
 #define DECL(CLASS, BASE)                                                      \
   case Decl::CLASS:                                                            \
     if (!getDerived().Traverse##CLASS##Decl(static_cast<CLASS##Decl *>(D)))    \
       return false;                                                            \
     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;
 }
 
 #undef DISPATCH
 
 template <typename Derived>
 bool RecursiveASTVisitor<Derived>::TraverseNestedNameSpecifier(
     NestedNameSpecifier *NNS) {
#define DEF_TRAVERSE_DECL(DECL, CODE)                                          \
   bool RecursiveASTVisitor<Derived>::Traverse##DECL(DECL *D) {                 \
     bool ShouldVisitChildren = true;                                           \
     bool ReturnValue = true;                                                   \
     if (!getDerived().shouldTraversePostOrder())                               \
       TRY_TO(WalkUpFrom##DECL(D));                                             \
     { 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;                                                        \
   }
 
 DEF_TRAVERSE_DECL(AccessSpecDecl, {})
 
 DEF_TRAVERSE_DECL(BlockDecl, {