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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think this change should come with a warning in the release notes because it silently changes the behavior of existing matchers in a way that may be surprising to users. I think the behavior is correct in that these constructs are all implicit nodes that aren't really spelled in source, but users may have come to rely on the existing behavior. I will note that the default argument case is somewhat interesting to me because there is a defensible argument to be made that the expression is spelled in source (it's spelled at the location of the function declaration rather than at the call site) and that same logic applies to explicitly defaulted function declarations. However, given the goal of trying to model this on the syntactic form of what the user wrote, I think the proposed behavior is correct.



================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3115
+
+  if (Finder->getASTContext().getParentMapContext().getTraversalKind() !=
+          TK_AsIs &&
----------------
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.)


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4056
                           unsigned, N) {
-  return Node.getNumArgs() == N;
+  auto NumArgs = Node.getNumArgs();
+  if (Finder->getASTContext().getParentMapContext().getTraversalKind() ==
----------------
Please spell out the type.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4061
+  while (NumArgs) {
+    const auto *Arg = Node.getArg(NumArgs - 1);
+    if (!isa<CXXDefaultArgExpr>(Arg))
----------------
Likewise here.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4084
+    return false;
+  const auto *Arg = Node.getArg(N);
+  if (Finder->getASTContext().getParentMapContext().getTraversalKind() !=
----------------
And here.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4311
                           internal::Matcher<Expr>, InnerMatcher) {
+  auto TK = Finder->getASTContext().getParentMapContext().getTraversalKind();
   for (const Expr *Arg : Node.arguments()) {
----------------
Spell the type out?


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:5466
+            TK_AsIs &&
+        FD->isDefaulted())
+      return false;
----------------
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.



================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:7078
+            TK_AsIs &&
+        FD->isDefaulted())
+      return false;
----------------
Same question here about why defaulted matters as above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90984