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

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D88275#2319290 , @ymandel wrote:

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



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

I didn't go to the coffee break, but am in the afters hangout.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88275

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D88275#2316484 , @aaron.ballman 
wrote:

> Thank you for your patience while we sort this out. I've pinged @steveire 
> off-list, so hopefully he'll respond with his thoughts. If we don't hear from 
> Stephen in the next week or so, I think we should proceed with your proposed 
> patch to get you unblocked (adding one more "ignore implicit" variant isn't 
> the end of the world, I just want us to be thoughtful about whether we want 
> to add new matchers in this space or to work on the traversal mode instead).

Sure!  Thanks for reaching out to Stephen.  This sounds good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88275

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D88275#2313342 , @ymandel wrote:

> In D88275#2305989 , @aaron.ballman 
> wrote:
>
>> In D88275#2303283 , @ymandel wrote:
>>
 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.
>>>
>>> Aaron, I appreciate this concern, but I would argue that this matcher isn't 
>>> making things any worse. We already have the various `ignoringImplicit` 
>>> matchers, and this new one simply parallels those, but for parents. So, it 
>>> is in some sense "completing" an existing API, which together is an 
>>> alternative to  `traverse`.
>>
>> I'm not certain I agree with that reasoning because you can extend it to 
>> literally any match that may interact with implicit nodes, which is the 
>> whole point to the spelled in source traversal mode. I'm not certain it's a 
>> good design for us to continue to add one-off ways to ignore implicit nodes.
>
> I appreciate your point, but I'm personally inclined to allow progress while 
> we figure these larger issues out.  That said, I'm in no rush and would like 
> a solution that we're both happy with. How do you propose we proceed, 
> especially given that `traverse` does not currently support this case? It 
> seems that progress is in part blocked on hearing back from steveire, but 
> it's been over a week since you added him to the review thread. Is there some 
> other way to ping him?

Thank you for your patience while we sort this out. I've pinged @steveire 
off-list, so hopefully he'll respond with his thoughts. If we don't hear from 
Stephen in the next week or so, I think we should proceed with your proposed 
patch to get you unblocked (adding one more "ignore implicit" variant isn't the 
end of the world, I just want us to be thoughtful about whether we want to add 
new matchers in this space or to work on the traversal mode instead).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88275

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D88275#2305989 , @aaron.ballman 
wrote:

> In D88275#2303283 , @ymandel wrote:
>
>>> 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.
>>
>> Aaron, I appreciate this concern, but I would argue that this matcher isn't 
>> making things any worse. We already have the various `ignoringImplicit` 
>> matchers, and this new one simply parallels those, but for parents. So, it 
>> is in some sense "completing" an existing API, which together is an 
>> alternative to  `traverse`.
>
> I'm not certain I agree with that reasoning because you can extend it to 
> literally any match that may interact with implicit nodes, which is the whole 
> point to the spelled in source traversal mode. I'm not certain it's a good 
> design for us to continue to add one-off ways to ignore implicit nodes.

I appreciate your point, but I'm personally inclined to allow progress while we 
figure these larger issues out.  That said, I'm in no rush and would like a 
solution that we're both happy with. How do you propose we proceed, especially 
given that `traverse` does not currently support this case? It seems that 
progress is in part blocked on hearing back from steveire, but it's been over a 
week since you added him to the review thread. Is there some other way to ping 
him?

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88275

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D88275#2303283 , @ymandel wrote:

>> 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.
>
> Aaron, I appreciate this concern, but I would argue that this matcher isn't 
> making things any worse. We already have the various `ignoringImplicit` 
> matchers, and this new one simply parallels those, but for parents. So, it is 
> in some sense "completing" an existing API, which together is an alternative 
> to  `traverse`.

I'm not certain I agree with that reasoning because you can extend it to 
literally any match that may interact with implicit nodes, which is the whole 
point to the spelled in source traversal mode. I'm not certain it's a good 
design for us to continue to add one-off ways to ignore implicit nodes.

> We should decide our general policy on these implict matchers vs the traverse 
> matchers.

I agree.

> Personally, I view `traverse` as an experimental feature that we're still 
> figuring out and so would prefer that it not block progress on new matchers. 
> But, I'm open to discussion. I can implement this one in my own codebase in 
> the meantime if this requires longer discussion (that is, it's not blocking 
> me, fwiw).

FWIW, I think of `traverse` as experimental as well. I can see the argument for 
not letting an experimental feature block progress on new matchers, too. I'm 
mostly worried about the case where we add the new matches and keep the 
`traverse` API, but they have subtly different behaviors and no clear policy on 
when to use which form.

> Also, I don't believe that traverse work in this case. When I change the test 
> to use `traverse`, it fails:
>
>   TEST(HasParentIgnoringImplicit, TraverseMatchesExplicitParents) {
> std::string Input = R"cc(
>   float f() {
>   int x = 3;
>   int y = 3.0;
>   return y;
>   }
> )cc";
>  // ---> Passes because there are no implicit parents.
> EXPECT_TRUE(matches(
> Input, integerLiteral(traverse(TK_IgnoreImplicitCastsAndParentheses,
>hasParent(varDecl());
> // Fails
> EXPECT_TRUE(
> matches(Input, 
> declRefExpr(traverse(TK_IgnoreImplicitCastsAndParentheses,
> hasParent(returnStmt());
> // Fails
> EXPECT_TRUE(
> matches(Input, 
> floatLiteral(traverse(TK_IgnoreImplicitCastsAndParentheses,
>  hasParent(varDecl());
>   }

Interesting catch and not the behavior I would expect from `traverse`. 
@steveire, would you consider this to be a bug in the traversal behavior?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88275

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

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

Aaron, I appreciate this concern, but I would argue that this matcher isn't 
making things any worse. We already have the various `ignoringImplicit` 
matchers, and this new one simply parallels those, but for parents. So, it is 
in some sense "completing" an existing API, which together is an alternative to 
 `traverse`.

We should decide our general policy on these implict matchers vs the traverse 
matchers. Personally, I view `traverse` as an experimental feature that we're 
still figuring out and so would prefer that it not block progress on new 
matchers. But, I'm open to discussion. I can implement this one in my own 
codebase in the meantime if this requires longer discussion (that is, it's not 
blocking me, fwiw).

Also, I don't believe that traverse work in this case. When I change the test 
to use `traverse`, it fails:

  TEST(HasParentIgnoringImplicit, TraverseMatchesExplicitParents) {
std::string Input = R"cc(
  float f() {
  int x = 3;
  int y = 3.0;
  return y;
  }
)cc";
 // ---> Passes because there are not implicit parents.
EXPECT_TRUE(matches(
Input, integerLiteral(traverse(TK_IgnoreImplicitCastsAndParentheses,
   hasParent(varDecl());
  
EXPECT_TRUE(
matches(Input, 
declRefExpr(traverse(TK_IgnoreImplicitCastsAndParentheses,
hasParent(returnStmt());
EXPECT_TRUE(
matches(Input, 
floatLiteral(traverse(TK_IgnoreImplicitCastsAndParentheses,
 hasParent(varDecl());
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88275

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

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

In D88275#2295413 , @ymandel wrote:

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 294363.
ymandel added a comment.

restored changes to unrelated parts of html docs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88275

Files:
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -3190,6 +3190,26 @@
compoundStmt(hasParent(recordDecl();
 }
 
+TEST(HasParentIgnoringImplicit, MatchesExplicitParents) {
+  std::string Input = R"cc(
+float f() {
+int x = 3;
+int y = 3.0;
+return y;
+}
+  )cc";
+  EXPECT_TRUE(
+  matches(Input, declRefExpr(hasParentIgnoringImplicit(returnStmt();
+  EXPECT_TRUE(
+  matches(Input, floatLiteral(hasParentIgnoringImplicit(varDecl();
+  EXPECT_TRUE(
+  matches(Input, integerLiteral(hasParentIgnoringImplicit(varDecl();
+
+  // Make sure it only ignores implicit ancestors.
+  EXPECT_TRUE(
+  notMatches(Input, integerLiteral(hasParentIgnoringImplicit(declStmt();
+}
+
 TEST(HasParent, NoDuplicateParents) {
   class HasDuplicateParents : public BoundNodesCallback {
   public:
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -311,6 +311,7 @@
   REGISTER_MATCHER(hasOverloadedOperatorName);
   REGISTER_MATCHER(hasParameter);
   REGISTER_MATCHER(hasParent);
+  REGISTER_MATCHER(hasParentIgnoringImplicit);
   REGISTER_MATCHER(hasQualifier);
   REGISTER_MATCHER(hasRHS);
   REGISTER_MATCHER(hasRangeInit);
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -544,9 +544,14 @@
 // don't invalidate any iterators.
 if (ResultCache.size() > MaxMemoizationEntries)
   ResultCache.clear();
-if (MatchMode == AncestorMatchMode::AMM_ParentOnly)
+switch (MatchMode) {
+case AncestorMatchMode::AMM_ParentOnly:
   return matchesParentOf(Node, Matcher, Builder);
-return matchesAnyAncestorOf(Node, Ctx, Matcher, Builder);
+case AncestorMatchMode::AMM_FirstExplicitOnly:
+  return matchesFirstExplicitAncestorOf(Node, Matcher, Builder);
+case AncestorMatchMode::AMM_All:
+  return matchesAnyAncestorOf(Node, Ctx, Matcher, Builder);
+}
   }
 
   // Matches all registered matchers on the given node and calls the
@@ -714,6 +719,33 @@
 return false;
   }
 
+  // Returns whether the first explicit (`Expr`) ancestor of \p Node matches \p
+  // Matcher. That is, like matchesParentOf but skipping implicit parents.
+  // Unlike matchesAnyAncestorOf there's no memoization: it doesn't save much.
+  bool matchesFirstExplicitAncestorOf(const DynTypedNode ,
+  const DynTypedMatcher ,
+  BoundNodesTreeBuilder *Builder) {
+for (const auto  : ActiveASTContext->getParents(Node)) {
+  if (const auto *E = Parent.get())
+// If the parent is an implicit node, match on *its* parents
+// instead. Use DFS, since we expect that expressions are relatively
+// shallow.
+if (clang::isa(E) || clang::isa(E) ||
+clang::isa(E) ||
+clang::isa(E)) {
+  if (matchesFirstExplicitAncestorOf(Parent, Matcher, Builder))
+return true;
+  continue;
+}
+  BoundNodesTreeBuilder BuilderCopy = *Builder;
+  if (Matcher.matches(Parent, this, )) {
+*Builder = std::move(BuilderCopy);
+return true;
+  }
+}
+return false;
+  }
+
   // Returns whether an ancestor of \p Node matches \p Matcher.
   //
   // The order of matching (which can lead to different nodes being bound in
Index: clang/include/clang/ASTMatchers/ASTMatchersInternal.h
===
--- clang/include/clang/ASTMatchers/ASTMatchersInternal.h
+++ clang/include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -985,7 +985,11 @@
 AMM_All,
 
 /// Direct parent only.
-AMM_ParentOnly
+AMM_ParentOnly,
+
+/// Considers the first non-implicit `Expr` ancestor. Intuitively, like
+/// `ignoringImplicit` for matching parents.
+AMM_FirstExplicitOnly
   };
 
   virtual ~ASTMatchFinder() = default;
@@ -1515,6 +1519,33 @@
   }
 };
 
+/// Matches \c Stmt nodes that 

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In 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.

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

thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88275

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88275

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 294348.
ymandel added a comment.

Fixed to use more standard type adaptors. Registration now works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88275

Files:
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -3190,6 +3190,26 @@
compoundStmt(hasParent(recordDecl();
 }
 
+TEST(HasParentIgnoringImplicit, MatchesExplicitParents) {
+  std::string Input = R"cc(
+float f() {
+int x = 3;
+int y = 3.0;
+return y;
+}
+  )cc";
+  EXPECT_TRUE(
+  matches(Input, declRefExpr(hasParentIgnoringImplicit(returnStmt();
+  EXPECT_TRUE(
+  matches(Input, floatLiteral(hasParentIgnoringImplicit(varDecl();
+  EXPECT_TRUE(
+  matches(Input, integerLiteral(hasParentIgnoringImplicit(varDecl();
+
+  // Make sure it only ignores implicit ancestors.
+  EXPECT_TRUE(
+  notMatches(Input, integerLiteral(hasParentIgnoringImplicit(declStmt();
+}
+
 TEST(HasParent, NoDuplicateParents) {
   class HasDuplicateParents : public BoundNodesCallback {
   public:
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -311,6 +311,7 @@
   REGISTER_MATCHER(hasOverloadedOperatorName);
   REGISTER_MATCHER(hasParameter);
   REGISTER_MATCHER(hasParent);
+  REGISTER_MATCHER(hasParentIgnoringImplicit);
   REGISTER_MATCHER(hasQualifier);
   REGISTER_MATCHER(hasRHS);
   REGISTER_MATCHER(hasRangeInit);
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -544,9 +544,14 @@
 // don't invalidate any iterators.
 if (ResultCache.size() > MaxMemoizationEntries)
   ResultCache.clear();
-if (MatchMode == AncestorMatchMode::AMM_ParentOnly)
+switch (MatchMode) {
+case AncestorMatchMode::AMM_ParentOnly:
   return matchesParentOf(Node, Matcher, Builder);
-return matchesAnyAncestorOf(Node, Ctx, Matcher, Builder);
+case AncestorMatchMode::AMM_FirstExplicitOnly:
+  return matchesFirstExplicitAncestorOf(Node, Matcher, Builder);
+case AncestorMatchMode::AMM_All:
+  return matchesAnyAncestorOf(Node, Ctx, Matcher, Builder);
+}
   }
 
   // Matches all registered matchers on the given node and calls the
@@ -714,6 +719,33 @@
 return false;
   }
 
+  // Returns whether the first explicit (`Expr`) ancestor of \p Node matches \p
+  // Matcher. That is, like matchesParentOf but skipping implicit parents.
+  // Unlike matchesAnyAncestorOf there's no memoization: it doesn't save much.
+  bool matchesFirstExplicitAncestorOf(const DynTypedNode ,
+  const DynTypedMatcher ,
+  BoundNodesTreeBuilder *Builder) {
+for (const auto  : ActiveASTContext->getParents(Node)) {
+  if (const auto *E = Parent.get())
+// If the parent is an implicit node, match on *its* parents
+// instead. Use DFS, since we expect that expressions are relatively
+// shallow.
+if (clang::isa(E) || clang::isa(E) ||
+clang::isa(E) ||
+clang::isa(E)) {
+  if (matchesFirstExplicitAncestorOf(Parent, Matcher, Builder))
+return true;
+  continue;
+}
+  BoundNodesTreeBuilder BuilderCopy = *Builder;
+  if (Matcher.matches(Parent, this, )) {
+*Builder = std::move(BuilderCopy);
+return true;
+  }
+}
+return false;
+  }
+
   // Returns whether an ancestor of \p Node matches \p Matcher.
   //
   // The order of matching (which can lead to different nodes being bound in
Index: clang/include/clang/ASTMatchers/ASTMatchersInternal.h
===
--- clang/include/clang/ASTMatchers/ASTMatchersInternal.h
+++ clang/include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -985,7 +985,11 @@
 AMM_All,
 
 /// Direct parent only.
-AMM_ParentOnly
+AMM_ParentOnly,
+
+/// Considers the first non-implicit `Expr` ancestor. Intuitively, like
+/// `ignoringImplicit` for matching parents.
+AMM_FirstExplicitOnly
   };
 
   virtual ~ASTMatchFinder() = default;
@@ -1515,6 +1519,33 @@
   }
 };
 
+/// Matches \c 

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 294312.
ymandel added a comment.

update dynamic registry and the ast matcher doc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88275

Files:
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -3190,6 +3190,26 @@
compoundStmt(hasParent(recordDecl();
 }
 
+TEST(HasParentIgnoringImplicit, MatchesExplicitParents) {
+  std::string Input = R"cc(
+float f() {
+int x = 3;
+int y = 3.0;
+return y;
+}
+  )cc";
+  EXPECT_TRUE(
+  matches(Input, declRefExpr(hasParentIgnoringImplicit(returnStmt();
+  EXPECT_TRUE(
+  matches(Input, floatLiteral(hasParentIgnoringImplicit(varDecl();
+  EXPECT_TRUE(
+  matches(Input, integerLiteral(hasParentIgnoringImplicit(varDecl();
+
+  // Make sure it only ignores implicit ancestors.
+  EXPECT_TRUE(
+  notMatches(Input, integerLiteral(hasParentIgnoringImplicit(declStmt();
+}
+
 TEST(HasParent, NoDuplicateParents) {
   class HasDuplicateParents : public BoundNodesCallback {
   public:
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -311,6 +311,7 @@
   REGISTER_MATCHER(hasOverloadedOperatorName);
   REGISTER_MATCHER(hasParameter);
   REGISTER_MATCHER(hasParent);
+  REGISTER_MATCHER(hasParentIgnoringImplicit);
   REGISTER_MATCHER(hasQualifier);
   REGISTER_MATCHER(hasRHS);
   REGISTER_MATCHER(hasRangeInit);
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -544,9 +544,14 @@
 // don't invalidate any iterators.
 if (ResultCache.size() > MaxMemoizationEntries)
   ResultCache.clear();
-if (MatchMode == AncestorMatchMode::AMM_ParentOnly)
+switch (MatchMode) {
+case AncestorMatchMode::AMM_ParentOnly:
   return matchesParentOf(Node, Matcher, Builder);
-return matchesAnyAncestorOf(Node, Ctx, Matcher, Builder);
+case AncestorMatchMode::AMM_FirstExplicitOnly:
+  return matchesFirstExplicitAncestorOf(Node, Matcher, Builder);
+case AncestorMatchMode::AMM_All:
+  return matchesAnyAncestorOf(Node, Ctx, Matcher, Builder);
+}
   }
 
   // Matches all registered matchers on the given node and calls the
@@ -714,6 +719,33 @@
 return false;
   }
 
+  // Returns whether the first explicit (`Expr`) ancestor of \p Node matches \p
+  // Matcher. That is, like matchesParentOf but skipping implicit parents.
+  // Unlike matchesAnyAncestorOf there's no memoization: it doesn't save much.
+  bool matchesFirstExplicitAncestorOf(const DynTypedNode ,
+  const DynTypedMatcher ,
+  BoundNodesTreeBuilder *Builder) {
+for (const auto  : ActiveASTContext->getParents(Node)) {
+  if (const auto *E = Parent.get())
+// If the parent is an implicit node, match on *its* parents
+// instead. Use DFS, since we expect that expressions are relatively
+// shallow.
+if (clang::isa(E) || clang::isa(E) ||
+clang::isa(E) ||
+clang::isa(E)) {
+  if (matchesFirstExplicitAncestorOf(Parent, Matcher, Builder))
+return true;
+  continue;
+}
+  BoundNodesTreeBuilder BuilderCopy = *Builder;
+  if (Matcher.matches(Parent, this, )) {
+*Builder = std::move(BuilderCopy);
+return true;
+  }
+}
+return false;
+  }
+
   // Returns whether an ancestor of \p Node matches \p Matcher.
   //
   // The order of matching (which can lead to different nodes being bound in
Index: clang/include/clang/ASTMatchers/ASTMatchersInternal.h
===
--- clang/include/clang/ASTMatchers/ASTMatchersInternal.h
+++ clang/include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -985,7 +985,11 @@
 AMM_All,
 
 /// Direct parent only.
-AMM_ParentOnly
+AMM_ParentOnly,
+
+/// Considers the first non-implicit `Expr` ancestor. Intuitively, like
+/// `ignoringImplicit` for matching parents.
+AMM_FirstExplicitOnly
   };
 
   virtual ~ASTMatchFinder() = default;
@@ -1515,6 +1519,26 @@
   }
 };
 
+/// Matches nodes of type \c T that 

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel created this revision.
ymandel added a reviewer: gribozavr2.
Herald added a project: clang.
ymandel requested review of this revision.

This matcher combines the features of the `hasParent` matcher with
skip-implicit-expression-nodes behavior of `ignoringImplicit`. It finds the
first ancestor which is reachable through only implict expression nodes and
matches the given argument matcher.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88275

Files:
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -3190,6 +3190,26 @@
compoundStmt(hasParent(recordDecl();
 }
 
+TEST(HasParentIgnoringImplicit, MatchesExplicitParents) {
+  std::string Input = R"cc(
+float f() {
+int x = 3;
+int y = 3.0;
+return y;
+}
+  )cc";
+  EXPECT_TRUE(
+  matches(Input, declRefExpr(hasParentIgnoringImplicit(returnStmt();
+  EXPECT_TRUE(
+  matches(Input, floatLiteral(hasParentIgnoringImplicit(varDecl();
+  EXPECT_TRUE(
+  matches(Input, integerLiteral(hasParentIgnoringImplicit(varDecl();
+
+  // Make sure it only ignores implicit ancestors.
+  EXPECT_TRUE(
+  notMatches(Input, integerLiteral(hasParentIgnoringImplicit(declStmt();
+}
+
 TEST(HasParent, NoDuplicateParents) {
   class HasDuplicateParents : public BoundNodesCallback {
   public:
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -544,9 +544,14 @@
 // don't invalidate any iterators.
 if (ResultCache.size() > MaxMemoizationEntries)
   ResultCache.clear();
-if (MatchMode == AncestorMatchMode::AMM_ParentOnly)
+switch (MatchMode) {
+case AncestorMatchMode::AMM_ParentOnly:
   return matchesParentOf(Node, Matcher, Builder);
-return matchesAnyAncestorOf(Node, Ctx, Matcher, Builder);
+case AncestorMatchMode::AMM_FirstExplicitOnly:
+  return matchesFirstExplicitAncestorOf(Node, Matcher, Builder);
+case AncestorMatchMode::AMM_All:
+  return matchesAnyAncestorOf(Node, Ctx, Matcher, Builder);
+}
   }
 
   // Matches all registered matchers on the given node and calls the
@@ -714,6 +719,33 @@
 return false;
   }
 
+  // Returns whether the first explicit (`Expr`) ancestor of \p Node matches \p
+  // Matcher. That is, like matchesParentOf but skipping implicit parents.
+  // Unlike matchesAnyAncestorOf there's no memoization: it doesn't save much.
+  bool matchesFirstExplicitAncestorOf(const DynTypedNode ,
+  const DynTypedMatcher ,
+  BoundNodesTreeBuilder *Builder) {
+for (const auto  : ActiveASTContext->getParents(Node)) {
+  if (const auto *E = Parent.get())
+// If the parent is an implicit node, match on *its* parents
+// instead. Use DFS, since we expect that expressions are relatively
+// shallow.
+if (clang::isa(E) || clang::isa(E) ||
+clang::isa(E) ||
+clang::isa(E)) {
+  if (matchesFirstExplicitAncestorOf(Parent, Matcher, Builder))
+return true;
+  continue;
+}
+  BoundNodesTreeBuilder BuilderCopy = *Builder;
+  if (Matcher.matches(Parent, this, )) {
+*Builder = std::move(BuilderCopy);
+return true;
+  }
+}
+return false;
+  }
+
   // Returns whether an ancestor of \p Node matches \p Matcher.
   //
   // The order of matching (which can lead to different nodes being bound in
Index: clang/include/clang/ASTMatchers/ASTMatchersInternal.h
===
--- clang/include/clang/ASTMatchers/ASTMatchersInternal.h
+++ clang/include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -985,7 +985,11 @@
 AMM_All,
 
 /// Direct parent only.
-AMM_ParentOnly
+AMM_ParentOnly,
+
+/// Considers the first non-implicit `Expr` ancestor. Intuitively, like
+/// `ignoringImplicit` for matching parents.
+AMM_FirstExplicitOnly
   };
 
   virtual ~ASTMatchFinder() = default;
@@ -1515,6 +1519,26 @@
   }
 };
 
+/// Matches nodes of type \c T that have a parent node for which the given inner
+/// matcher matches. If the parent is an implicit expression, considers the
+/// parent's parent (and so on) instead, until an explicit parent is found. That
+/// is, looks for an ancestor that is separated from `Node` only by implicit
+/// expression nodes and matches `ParentMatcher`.
+template 
+class