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