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

[Stephen Kelly via Phabricator via cfe-commits]

steveire added inline comments.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3115
+
+  if (!Finder->isTraversalAsIs() && (*MatchIt)->isImplicit())
+    return false;
----------------
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.


================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4084
+    return false;
+  return InnerMatcher.matches(*Arg->IgnoreParenImpCasts(), Finder, Builder);
 }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90984