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

Fri Sep 25 12:59:05 PDT 2020

aaron.ballman added reviewers: steveire, klimek, sammccall.
aaron.ballman added a subscriber: steveire.
aaron.ballman added a comment.

In D88275#2295413 <https://reviews.llvm.org/D88275#2295413>, @ymandel wrote:

> In D88275#2295379 <https://reviews.llvm.org/D88275#2295379>, @aaron.ballman wrote:
>
>> This seems to be strongly related to the `TK_IgnoreUnlessSpelledInSource` traversal behavior; is there a reason you can't use that traversal mode with `hasParent()` (does it behave differently)?
>
> Aaron, that's a good point.  I hadn't realized that the traversal mode supported parent traversal. That said, even with it available, I have strong reservations about modal matching, especially given that it had severe issues when `TK_IgnoreUnlessSpelledInSource` went live a few months ago as the default setting. I don't know what the resolution of those discussions were, but I thought we simply agreed to restore the default, and left fixing the underlying issues as future work.

That is the resolution we came to.

> But, even it if worked as desired, to achieve the same effect, you would have to restore the current mode as well once you're reached your desired destination. e.g. inspired by the tests, given some matcher `m`,
>
>   integerLiteral(hasParentIgnoringImplicit(varDecl(m)))
>
> becomes
>
>   integerLiteral(traversal(TK_IgnoreUnlessSpelledInSource, hasParent(varDecl(traversal(TK_AsIs, m)))))
>
> which seems a lot worse to me.

It's certainly more wordy, but do you envision this matcher functionality to be needed so often that we need to introduce a second way to achieve the same thing?

> We could however implement this new one in terms of `traverse`, but that would go back to the question of whether it is working correctly and also gets somewhat tricky (specifically, restoring the ambient traversal mode).  Do you know the current status?

I'm adding @steveire in case he has any updates or opinions, but my understanding of the current status is that the defaults have been restored but no further work has been done for implicit node matching.

I'm not concerned about the basic idea behind the proposed matcher, I'm only worried we're making AST matching more confusing by having two different ways of inconsistently accomplishing the same-ish thing.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88275