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