[PATCH] D135690: [ASTMatchers] Add matcher for functions that are effectively inline
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 12 05:30:22 PDT 2022
aaron.ballman added a comment.
Thanks for the new matcher, can you also add a release note for the addition? One question I have is: what's the need for adding this matcher? Do you plan to use it for some in-tree needs? We usually only add new matchers where there's an immediate need for them because of how expensive AST matchers are to compile (and each matcher adds a fair number of template instantiations to the final binary as well, so there's a bit of runtime cost too).
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:7787-7788
+/// void f() {}
+/// void g();
+/// };
+/// \endcode
----------------
I think it'd be interesting to show some other cases here -- like when the inline keyword is specified on a declaration without a visible definition or when the inline keyword is used on a declaration with a non-inline definition. (And update the comment below accordingly with what is matched.)
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:7791
+/// functionDecl(isEffectivelyInline()) will match f().
+AST_MATCHER(FunctionDecl, isEffectivelyInline) { return Node.isInlined(); }
+
----------------
Trass3r wrote:
> Is there any value in adding variables?
> I guess that would mainly target constexpr vars, for which there is `isConstexpr`.
I don't see a need for covering variables yet, but I wouldn't be opposed if there was a use case.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchersMacros.h:96
namespace internal { \
- class matcher_##DefineMatcher##Matcher \
+ class matcher_##DefineMatcher##Matcher final \
: public ::clang::ast_matchers::internal::MatcherInterface<Type> { \
----------------
Trass3r wrote:
> ziqingluo-90 wrote:
> > I was wondering if this change is necessary. This definition is so general that it could affect a massive matchers. So any change to it should be very careful and unnecessary changes may be avoided.
> >
> > Other than that, this patch looks good to me.
> Just came across it. Since it's a macro-internal class I couldn't see any potential breakage. Nobody should inherit from that. But I can take it out.
I'd remove it as it's unrelated to the changes in the patch, but it might be a reasonable NFC change (especially if there's some positive impact to the change in terms of codegen).
================
Comment at: clang/lib/ASTMatchers/Dynamic/Registry.cpp:435
REGISTER_MATCHER(isInline);
+ REGISTER_MATCHER(isEffectivelyInline);
REGISTER_MATCHER(isInstanceMessage);
----------------
This should be kept in alphabetical order.
================
Comment at: clang/unittests/ASTMatchers/ASTMatchersInternalTest.cpp:310-311
+TEST(IsInlineMatcher, IsEffectivelyInline) {
+ EXPECT_TRUE(matches("class X { void f() {} void g(); };",
+ functionDecl(isEffectivelyInline(), hasName("f"))));
+ EXPECT_TRUE(matches("constexpr int f() { return 0; }",
----------------
I think you should also test that `g()` does not match and the additional tests I suggested in the documentation.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135690/new/
https://reviews.llvm.org/D135690
More information about the cfe-commits
mailing list