aaron.ballman added inline comments.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3115
+
+ if (Finder->getASTContext().getParentMapContext().getTraversalKind() !=
+ TK_AsIs &&
----------------
steveire wrote:
> aaron.ballman wrote:
> > Given how often this gets repeated (and what a mouthful it is), how about
> > adding a static helper function `ClassifyTraversal()` (or some such) that
> > returns the traversal kind given a `Finder`? (Alternatively,
> > `isTraversalAsIs()` or some specific traversal behavior.)
> Done in D91144.
Thanks! Just an FYI, but it looks like the changes in D91144 are reflected in
this review. I don't mind but wanted to mention it in case it causes you
problems when trying to apply the commits to master.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:5466
+ TK_AsIs &&
+ FD->isDefaulted())
+ return false;
----------------
steveire wrote:
> aaron.ballman wrote:
> > In general (not specific to this matcher), I'd like to see some tests for
> > explicitly defaulted functions vs implicitly defaulted ones. e.g., I think
> > we should handle these differently, shouldn't we?
> > ```
> > struct S {}; // Has implicitly defaulted constructor
> > struct T {
> > T() = default; // Has explicitly defaulted constructor
> > };
> > ```
> > Specific to this matcher, I am a bit concerned about the behavior of
> > answering "isDefinition" based on whether a function is defaulted or not.
> > The user has already gotten some `FunctionDecl` object, so why would the
> > implicit vs explicit nature of the declaration matter for deciding whether
> > the node is a definition? The behavior with `hasBody` makes sense to me
> > because the body may be implicitly provided, but whether something is or is
> > not a definition is a different kind of question.
> >
> I agree for both `isDefinition` and `isInline`. `hasBody` is the one to use
> for these cases.
Awesome, thanks for verifying!
================
Comment at: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:2368
NonTrivial m_nt;
- HasCtorInits() : NoSpecialMethods(), m_i(42) {}
};
----------------
Was this originally testing behavior with explicitly initializing an implicitly
generated ctor (since that's also an implicit node)? Perhaps this change should
be reverted and we make a second test?
================
Comment at: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:2422
+ hasType(cxxRecordDecl(hasName("NoSpecialMethods"))),
+ callee(cxxMethodDecl(isCopyAssignmentOperator()).bind("callTarget")));
+ EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
----------------
You can drop the `.bind()` I believe.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90984/new/
https://reviews.llvm.org/D90984
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
