[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->isTraversalAsIs() && (*MatchIt)->isImplicit())
+    return false;
----------------
steveire wrote:
> aaron.ballman wrote:
> > If the traversal is not `AsIs`, that doesn't mean it's `IgnoreUnlessSpelledInSource` -- it could be `IgnoreImplicitCastsAndParentheses`, right? So I think the logic for some of these AST matchers is incorrect when in `IgnoreImplicitCastsAndParentheses` traversal mode and should be double-checked. We should probably add some extra test coverage for that mode.
> As far as I know, the problem of implicit nodes has been known for a long time. Adding calls to `IgnoreParenImpCasts()` in certain places like `hasArgument` was one attempt at making the implicit nodes less convenient for users. As far as I know, `IgnoreImplicitCastsAndParentheses` was just another attempt at the same thing. I must have discovered that by reading code history at some point. Both previous attempts didn't go far enough to actually solve the problem, but `IgnoreUnlessSpelledInSource` does go all the way, and `traverse` puts control in the hands of the user. D20801 at least seems to have been an attempt to put control back in the hands of the user. And it was a follow-up to D18243.
> 
> So, once this and D90982 are merged, I think it makes sense to start to remove `IgnoreImplicitCastsAndParentheses` entirely. It is legacy, incompletely useful and just causes some mess in the code. 
> 
> Two modes aught to be enough for anybody.
> As far as I know, IgnoreImplicitCastsAndParentheses was just another attempt at the same thing. I must have discovered that by reading code history at some point. 

I don't recall the history of this traversal mode, but I think you're correct.

> So, once this and D90982 are merged, I think it makes sense to start to remove IgnoreImplicitCastsAndParentheses entirely. It is legacy, incompletely useful and just causes some mess in the code.

I agree. I see at least two ways to proceed:

1) Change this patch to handle three modes so that we can land it without incorrect behavior, then do a follow-up patch to rip out the `IgnoreImplicitCastsAndParentheses` mode.
2) Rip out `IgnoreImplicitCastsAndParentheses` first and then land this patch as-is.

My preference is for #1 so that we don't land this patch in an awkward state (in case the removal of the other mode gets delayed for some reason). Given what I mention below, that shouldn't result in any undue churn, I'd hope.

> Two modes aught to be enough for anybody.

Heh, I would like to think that, but given that this enumeration is used for generic traversal of ASTs, I'm less convinced that two modes will be all we ever want to support. I think it's a bit more future-proof to add a helper method along the lines of `isTraversalIgnoringImplicitNodes()` rather than using `!isTraversalAsIs()`.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4054
                           unsigned, N) {
-  return Node.getNumArgs() == N;
+  auto NumArgs = Node.getNumArgs();
+  if (Finder->isTraversalAsIs())
----------------
`auto` -> `unsigned` please


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4058
+  while (NumArgs) {
+    const auto *Arg = Node.getArg(NumArgs - 1);
+    if (!isa<CXXDefaultArgExpr>(Arg))
----------------
`auto` -> `const Expr *`  or just get rid of the local variable entirely by sinking the initialization into the `isa<>`.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4081
+    return false;
+  const auto *Arg = Node.getArg(N);
+  if (!Finder->isTraversalAsIs() && isa<CXXDefaultArgExpr>(Arg))
----------------
`const auto *` -> `const Expr *`


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4084
+    return false;
+  return InnerMatcher.matches(*Arg->IgnoreParenImpCasts(), Finder, Builder);
 }
----------------
steveire wrote:
> aaron.ballman wrote:
> > Huh, I never noticed that we implicitly are ignoring parens and implicit casts for this (without checking the traversal mode or documenting the behavior!). That seems less-than-ideal in some ways. (No change required, I only noticed it because it made me think through whether we need it on the `isa<>` check above, which we don't.)
> Yes, I think calls to `ignore*` like this within matcher implementations should be removed, giving the user control instead.
I agree. I'm a little bit worried about what will break when we make the change, but I think that model is the more defensible one. (It looks like there are a handful of similar cases in ASTMatchers.h, so we should try to hit them all).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90984