[PATCH] D89743: Support Attr in DynTypedNode and ASTMatchers.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a comment.

This is *awesome*, thank you so much for working on it! One question I have is: as it stands, this is not a particularly useful matcher because it can only be used to say "yup, there's an attribute" -- are you planning to extend this functionality so that you can do something like `attr(hasName("foo"))`, or specify the syntax the attribute uses, `isImplicit()`, etc?



================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6737
+///   struct [[nodiscard]] Foo{};
+///   void bar(int * __attribute__((nonnull)) );
+/// \endcode
----------------
Can you also add an example using `__declspec()`?

Should I expect this to work on attributes added in other, perhaps surprising ways, like through `#pragma` or keywords?
```
// Some pragmas add attributes under the hood
#pragma omp declare simd 
int min(int a, int b) { return a < b ? a : b; }

// Other pragmas only exist to add attributes
#pragma clang attribute push (__attribute__((annotate("custom"))), apply_to = function)
void function(); // The function now has the annotate("custom") attribute
#pragma clang attribute pop

// Still other attributes come through keywords
alignas(16) int i;
```
If this is expected to match, we should document it more clearly.


================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:238
+    ScopedIncrement ScopedDepth(&CurrentDepth);
+    return (A == nullptr || traverse(*A));
+  }
----------------
Should we be properly handling `IgnoreUnlessSpelledInSource` for implicit attributes? e.g., `[[gnu::interrupt]] void func(int *i);` which gets two attributes (the `interrupt` attribute and an implicit `used` attribute).


================
Comment at: clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp:1879
+TEST_P(ASTMatchersTest, Attr) {
+  if (!GetParam().isCXX())
+    return;
----------------
Why?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89743/new/

https://reviews.llvm.org/D89743