[PATCH] D90984: Update matchers to be traverse-aware

[Aaron Ballman via Phabricator via cfe-commits]

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