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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

TL;DR Stephen's fix works; I'll drop this patch.

> 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?

Changing to `TK_IgnoreUnlessSpelledInSource` fixes the test, as you suggested. Thanks!

As for why I chose the former -- it was the closest match to what I was doing and the comments on the struct didn't guide me one way or another.

>> I have strong reservations about modal matching, especially given that it had severe issues
>
> I think "severe" is an overstatement.

I think it was an unhelpful word choice -- sorry about that.  I could go into more detail as to why I chose that, but without detail it's not a helpful description. Suffice it to say that we had multiple instances of suprising behaviour, which lost quite a few hours.

>> 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 ?

This is a longer discussion and not necessarily worth having on this review thread. Are you attending the dev meeting today? If so, want to chat during the coffee break? Otherwise, it seems like it could be helpful to schedule a meeting to discuss for some other time.

That said, I'm more inclined towards its use as a specialty operator, for situations like this, than as a general purpose tool for matchers. I can't say I much like the current regime of explicit `ignore` operators either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88275