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