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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits