[PATCH] D88275: [ASTMatchers] Add matcher `hasParentIgnoringImplicit`.

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

I don't get email notifications when I'm pinged on Phab for some reason. I didn't know about this until the email from Aaron.

  // Fails
  EXPECT_TRUE(
      matches(Input, declRefExpr(traverse(TK_IgnoreImplicitCastsAndParentheses,
                                          hasParent(returnStmt())))));

Why do you use `TK_IgnoreImplicitCastsAndParentheses` instead of `TK_IgnoreUnlessSpelledInSource`? All you seem to be doing is showing that the former is broken. I've always thought we should remove the former. `AsIs` and `IgnoreUnlessSpelledInSource` should be enough for anyone.

> @steveire, would you consider this [when using `TK_IgnoreImplicitCastsAndParentheses`] to be a bug in the traversal behavior?

It's probably a bug in how `TK_IgnoreImplicitCastsAndParentheses` is processed?

> I have strong reservations about modal matching, especially given that it had severe issues

I think "severe" is an overstatement.

> I'm only worried we're making AST matching more confusing by having two different ways of inconsistently accomplishing the same-ish thing.

The `traverse` matcher and `IgnoreUnlessSpelledInSource` were introduced so that matchers like `hasParentIgnoringImplicit` (and all the other `hasParentIgnoring*`) would never be needed (and so that the already-existing `ignore*` matchers would be needed only rarely). I agree with Aaron that it's not a good design to continue.

I think the existence of this new `hasParentIgnoringImplicit` matcher is further motivation that `IgnoreUnlessSpelledInSource` should be used more, especially when writing new matcher code. It is simpler. I get the impression people haven't tried it and prefer to write the complicated stuff instead.

I still don't see a problem with `traverse` being modal, but that fact seems to make you not use it, @ymandel ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88275