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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D89743#2341779 <https://reviews.llvm.org/D89743#2341779>, @aaron.ballman wrote:

> This is *awesome*, thank you so much for working on it!

Thanks for the comments - haven't addressed them yet, but wanted to clarify scope first.

> 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"

Yes definitely, I should have mentioned this...

My main goal here was to support it in DynTypedNode, as we have APIs (clangd::SelectionTree) that can only handle nodes that fit there.
But the common test pattern in ASTTypeTraitsTest used matchers, and I figured a basic matcher wasn't hard to add.

> 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?

I hadn't planned to - it definitely makes sense though I don't have any immediate use cases. I can do any of:

- leave as-is, waiting for someone to add matchers to make it useful
- scope down the patch to exclude the matcher (and write the ASTTypeTraitsTest another way)
- add some simple matcher like hasName (I guess in a followup patch) to make it minimally useful, with space for more

What do you think?



================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6737
+///   struct [[nodiscard]] Foo{};
+///   void bar(int * __attribute__((nonnull)) );
+/// \endcode
----------------
aaron.ballman wrote:
> 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.
I think the answer to all of these is yes, I'll update the docs.


================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:238
+    ScopedIncrement ScopedDepth(&CurrentDepth);
+    return (A == nullptr || traverse(*A));
+  }
----------------
aaron.ballman wrote:
> 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).
I think we should, I just wasn't thinking clearly about where that should go :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89743