[PATCH] D96113: [ASTMatchers] Fix hasParent while ignoring unwritten nodes

[Stephen Kelly via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe4d5f00093be: [ASTMatchers] Fix hasParent while ignoring 
unwritten nodes (authored by stephenkelly).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96113

Files:
  clang/include/clang/AST/ParentMapContext.h
  clang/lib/AST/ParentMapContext.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -2933,6 +2933,37 @@
 EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
 EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
   }
+  {
+auto M = ifStmt(hasParent(compoundStmt(hasParent(cxxForRangeStmt();
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_TRUE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
+  {
+auto M = cxxForRangeStmt(
+has(varDecl(hasName("i"), hasParent(cxxForRangeStmt();
+EXPECT_FALSE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_TRUE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
+  {
+auto M = cxxForRangeStmt(hasDescendant(varDecl(
+hasName("i"), hasParent(declStmt(hasParent(cxxForRangeStmt()));
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
+  {
+auto M = cxxForRangeStmt(hasRangeInit(declRefExpr(
+to(varDecl(hasName("arr"))), hasParent(cxxForRangeStmt();
+EXPECT_FALSE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_TRUE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
+
+  {
+auto M = cxxForRangeStmt(hasRangeInit(declRefExpr(
+to(varDecl(hasName("arr"))), hasParent(varDecl(hasParent(declStmt(
+ hasParent(cxxForRangeStmt();
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
 
   Code = R"cpp(
   struct Range {
@@ -3035,6 +3066,15 @@
 matchesConditionally(Code, traverse(TK_IgnoreUnlessSpelledInSource, M),
  true, {"-std=c++20"}));
   }
+  {
+auto M = cxxForRangeStmt(hasInitStatement(declStmt(
+hasSingleDecl(varDecl(hasName("a"))), hasParent(cxxForRangeStmt();
+EXPECT_TRUE(
+matchesConditionally(Code, traverse(TK_AsIs, M), true, {"-std=c++20"}));
+EXPECT_TRUE(
+matchesConditionally(Code, traverse(TK_IgnoreUnlessSpelledInSource, M),
+ true, {"-std=c++20"}));
+  }
 
   Code = R"cpp(
   struct Range {
@@ -3511,6 +3551,20 @@
forFunction(functionDecl(hasName("func13"))),
   langCxx20OrLater()));
 
+  EXPECT_TRUE(matches(Code,
+  traverse(TK_IgnoreUnlessSpelledInSource,
+   compoundStmt(hasParent(lambdaExpr(forFunction(
+   functionDecl(hasName("func13"))),
+  langCxx20OrLater()));
+
+  EXPECT_TRUE(matches(
+  Code,
+  traverse(TK_IgnoreUnlessSpelledInSource,
+   templateTypeParmDecl(hasName("TemplateType"),
+hasParent(lambdaExpr(forFunction(
+functionDecl(hasName("func14"))),
+  langCxx20OrLater()));
+
   EXPECT_TRUE(matches(
   Code,
   traverse(TK_IgnoreUnlessSpelledInSource,
@@ -3635,6 +3689,16 @@
 matchesConditionally(Code, traverse(TK_IgnoreUnlessSpelledInSource, M),
  true, {"-std=c++20"}));
   }
+  {
+auto M = cxxRewrittenBinaryOperator(
+hasLHS(expr(hasParent(cxxRewrittenBinaryOperator(,
+hasRHS(expr(hasParent(cxxRewrittenBinaryOperator();
+EXPECT_FALSE(
+matchesConditionally(Code, traverse(TK_AsIs, M), true, {"-std=c++20"}));
+EXPECT_TRUE(
+matchesConditionally(Code, traverse(TK_IgnoreUnlessSpelledInSource, M),
+ true, {"-std=c++20"}));
+  }
   {
 EXPECT_TRUE(matchesConditionally(
 Code,
Index: clang/lib/AST/ParentMapContext.cpp
===
--- clang/lib/AST/ParentMapContext.cpp
+++ clang/lib/AST/ParentMapContext.cpp
@@ -49,7 +49,17 @@
   return N;
 }
 
+template 
+std::tuple
+matchParents(const DynTypedNodeList ,
+ ParentMapContext::ParentMap *ParentMap);
+
+template  struct MatchParents;
+
 class ParentMapContext::ParentMap {
+
+  template  friend struct ::MatchParents;
+
   /// Contains parents of a node.
   using ParentVector = llvm::SmallVector;
 
@@ -117,11 +127,72 @@
 if 

[PATCH] D96113: [ASTMatchers] Fix hasParent while ignoring unwritten nodes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

In D96113#2569441 , @steveire wrote:

> Update

Thanks for the extra context, that's useful -- LGTM!




Comment at: clang/lib/AST/ParentMapContext.cpp:174
+{
+  auto AncestorNodes = matchParents(
+  ParentList, this);

steveire wrote:
> aaron.ballman wrote:
> > Not needing to be solved in this patch, but do we eventually need to do 
> > something for `ObjCForCollectionStmt` the same as we do for 
> > `CXXForRangeStmt`?
> Perhaps. I'm not familiar with it.
I'm not super familiar with it myself, but I know it uses `for (element in 
expression)` as the syntax which seems awfully similar to `for (auto decl : 
expr)` in C++ which is why I thought of it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96113

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

[PATCH] D96113: [ASTMatchers] Fix hasParent while ignoring unwritten nodes

[Stephen Kelly via Phabricator via cfe-commits]

steveire added inline comments.



Comment at: clang/lib/AST/ParentMapContext.cpp:174
+{
+  auto AncestorNodes = matchParents(
+  ParentList, this);

aaron.ballman wrote:
> Not needing to be solved in this patch, but do we eventually need to do 
> something for `ObjCForCollectionStmt` the same as we do for `CXXForRangeStmt`?
Perhaps. I'm not familiar with it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96113

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

[PATCH] D96113: [ASTMatchers] Fix hasParent while ignoring unwritten nodes

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 324396.
steveire added a comment.

Update


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96113

Files:
  clang/include/clang/AST/ParentMapContext.h
  clang/lib/AST/ParentMapContext.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -2933,6 +2933,37 @@
 EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
 EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
   }
+  {
+auto M = ifStmt(hasParent(compoundStmt(hasParent(cxxForRangeStmt();
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_TRUE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
+  {
+auto M = cxxForRangeStmt(
+has(varDecl(hasName("i"), hasParent(cxxForRangeStmt();
+EXPECT_FALSE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_TRUE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
+  {
+auto M = cxxForRangeStmt(hasDescendant(varDecl(
+hasName("i"), hasParent(declStmt(hasParent(cxxForRangeStmt()));
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
+  {
+auto M = cxxForRangeStmt(hasRangeInit(declRefExpr(
+to(varDecl(hasName("arr"))), hasParent(cxxForRangeStmt();
+EXPECT_FALSE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_TRUE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
+
+  {
+auto M = cxxForRangeStmt(hasRangeInit(declRefExpr(
+to(varDecl(hasName("arr"))), hasParent(varDecl(hasParent(declStmt(
+ hasParent(cxxForRangeStmt();
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
 
   Code = R"cpp(
   struct Range {
@@ -3035,6 +3066,15 @@
 matchesConditionally(Code, traverse(TK_IgnoreUnlessSpelledInSource, M),
  true, {"-std=c++20"}));
   }
+  {
+auto M = cxxForRangeStmt(hasInitStatement(declStmt(
+hasSingleDecl(varDecl(hasName("a"))), hasParent(cxxForRangeStmt();
+EXPECT_TRUE(
+matchesConditionally(Code, traverse(TK_AsIs, M), true, {"-std=c++20"}));
+EXPECT_TRUE(
+matchesConditionally(Code, traverse(TK_IgnoreUnlessSpelledInSource, M),
+ true, {"-std=c++20"}));
+  }
 
   Code = R"cpp(
   struct Range {
@@ -3511,6 +3551,20 @@
forFunction(functionDecl(hasName("func13"))),
   langCxx20OrLater()));
 
+  EXPECT_TRUE(matches(Code,
+  traverse(TK_IgnoreUnlessSpelledInSource,
+   compoundStmt(hasParent(lambdaExpr(forFunction(
+   functionDecl(hasName("func13"))),
+  langCxx20OrLater()));
+
+  EXPECT_TRUE(matches(
+  Code,
+  traverse(TK_IgnoreUnlessSpelledInSource,
+   templateTypeParmDecl(hasName("TemplateType"),
+hasParent(lambdaExpr(forFunction(
+functionDecl(hasName("func14"))),
+  langCxx20OrLater()));
+
   EXPECT_TRUE(matches(
   Code,
   traverse(TK_IgnoreUnlessSpelledInSource,
@@ -3635,6 +3689,16 @@
 matchesConditionally(Code, traverse(TK_IgnoreUnlessSpelledInSource, M),
  true, {"-std=c++20"}));
   }
+  {
+auto M = cxxRewrittenBinaryOperator(
+hasLHS(expr(hasParent(cxxRewrittenBinaryOperator(,
+hasRHS(expr(hasParent(cxxRewrittenBinaryOperator();
+EXPECT_FALSE(
+matchesConditionally(Code, traverse(TK_AsIs, M), true, {"-std=c++20"}));
+EXPECT_TRUE(
+matchesConditionally(Code, traverse(TK_IgnoreUnlessSpelledInSource, M),
+ true, {"-std=c++20"}));
+  }
   {
 EXPECT_TRUE(matchesConditionally(
 Code,
Index: clang/lib/AST/ParentMapContext.cpp
===
--- clang/lib/AST/ParentMapContext.cpp
+++ clang/lib/AST/ParentMapContext.cpp
@@ -49,7 +49,17 @@
   return N;
 }
 
+template 
+std::tuple
+matchParents(const DynTypedNodeList ,
+ ParentMapContext::ParentMap *ParentMap);
+
+template  struct MatchParents;
+
 class ParentMapContext::ParentMap {
+
+  template  friend struct ::MatchParents;
+
   /// Contains parents of a node.
   using ParentVector = llvm::SmallVector;
 
@@ -117,11 +127,72 @@
 if (Node.getNodeKind().hasPointerIdentity()) {
   auto ParentList =
   getDynNodeFromMap(Node.getMemoizationData(), PointerParents);
- 

[PATCH] D96113: [ASTMatchers] Fix hasParent while ignoring unwritten nodes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Can you add a bit more description to the review summary about what's being 
fixed? It helps the reviewers with context but it also helps when doing code 
archeology on changes.




Comment at: clang/lib/AST/ParentMapContext.cpp:138
+  // different intermediate nodes.
+  // Look up 4 levels for a cxxRewrittenBinaryOperator
+  auto RewrittenBinOpParentsList = ParentList;

Why four levels? (May be worth adding that to the comment so it seems like less 
of a magic number.)



Comment at: clang/lib/AST/ParentMapContext.cpp:140
+  auto RewrittenBinOpParentsList = ParentList;
+  auto I = 0;
+  while (RewrittenBinOpParentsList.size() == 1 && I++ < 4) {





Comment at: clang/lib/AST/ParentMapContext.cpp:143-145
+if (!S) {
+  break;
+}





Comment at: clang/lib/AST/ParentMapContext.cpp:152
+}
+if (RWBO->getLHS()->IgnoreUnlessSpelledInSource() != ChildExpr &&
+RWBO->getRHS()->IgnoreUnlessSpelledInSource() != ChildExpr)

If `ChildExpr` is null, we do a fair amount of work just to get to this point 
and break out of the `while` loop -- would it be more clear to add `ChildExpr 
&&` to the loop predicate?



Comment at: clang/lib/AST/ParentMapContext.cpp:160-162
+if (ParentExpr && ChildExpr) {
+  return AscendIgnoreUnlessSpelledInSource(ParentExpr, ChildExpr);
+}





Comment at: clang/lib/AST/ParentMapContext.cpp:174
+{
+  auto AncestorNodes = matchParents(
+  ParentList, this);

Not needing to be solved in this patch, but do we eventually need to do 
something for `ObjCForCollectionStmt` the same as we do for `CXXForRangeStmt`?



Comment at: clang/lib/AST/ParentMapContext.cpp:179-181
+  std::get(AncestorNodes)) {
+return std::get(AncestorNodes);
+  }





Comment at: clang/lib/AST/ParentMapContext.cpp:185
+  auto AncestorNodes =
+  matchParents(ParentList,
+ this);

Not needing to be solved in this patch, but do we need to eventually do 
something about `BlockExpr` the same as we do for `LambdaExpr`?



Comment at: clang/lib/AST/ParentMapContext.cpp:187-189
+  if (std::get(AncestorNodes)) {
+return std::get(AncestorNodes);
+  }





Comment at: clang/lib/AST/ParentMapContext.cpp:195-197
+  if (std::get(AncestorNodes)) {
+return std::get(AncestorNodes);
+  }





Comment at: clang/lib/AST/ParentMapContext.cpp:310-312
+  if (NextParentList.size() == 1) {
+return std::make_tuple(true, NodeList, TypedNode);
+  }




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96113

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

[PATCH] D96113: [ASTMatchers] Fix hasParent while ignoring unwritten nodes

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 321990.
steveire added a comment.

Update


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96113

Files:
  clang/include/clang/AST/ParentMapContext.h
  clang/lib/AST/ParentMapContext.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -2933,6 +2933,37 @@
 EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
 EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
   }
+  {
+auto M = ifStmt(hasParent(compoundStmt(hasParent(cxxForRangeStmt();
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_TRUE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
+  {
+auto M = cxxForRangeStmt(
+has(varDecl(hasName("i"), hasParent(cxxForRangeStmt();
+EXPECT_FALSE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_TRUE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
+  {
+auto M = cxxForRangeStmt(hasDescendant(varDecl(
+hasName("i"), hasParent(declStmt(hasParent(cxxForRangeStmt()));
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
+  {
+auto M = cxxForRangeStmt(hasRangeInit(declRefExpr(
+to(varDecl(hasName("arr"))), hasParent(cxxForRangeStmt();
+EXPECT_FALSE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_TRUE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
+
+  {
+auto M = cxxForRangeStmt(hasRangeInit(declRefExpr(
+to(varDecl(hasName("arr"))), hasParent(varDecl(hasParent(declStmt(
+ hasParent(cxxForRangeStmt();
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
 
   Code = R"cpp(
   struct Range {
@@ -3035,6 +3066,15 @@
 matchesConditionally(Code, traverse(TK_IgnoreUnlessSpelledInSource, M),
  true, {"-std=c++20"}));
   }
+  {
+auto M = cxxForRangeStmt(hasInitStatement(declStmt(
+hasSingleDecl(varDecl(hasName("a"))), hasParent(cxxForRangeStmt();
+EXPECT_TRUE(
+matchesConditionally(Code, traverse(TK_AsIs, M), true, {"-std=c++20"}));
+EXPECT_TRUE(
+matchesConditionally(Code, traverse(TK_IgnoreUnlessSpelledInSource, M),
+ true, {"-std=c++20"}));
+  }
 
   Code = R"cpp(
   struct Range {
@@ -3511,6 +3551,20 @@
forFunction(functionDecl(hasName("func13"))),
   langCxx20OrLater()));
 
+  EXPECT_TRUE(matches(Code,
+  traverse(TK_IgnoreUnlessSpelledInSource,
+   compoundStmt(hasParent(lambdaExpr(forFunction(
+   functionDecl(hasName("func13"))),
+  langCxx20OrLater()));
+
+  EXPECT_TRUE(matches(
+  Code,
+  traverse(TK_IgnoreUnlessSpelledInSource,
+   templateTypeParmDecl(hasName("TemplateType"),
+hasParent(lambdaExpr(forFunction(
+functionDecl(hasName("func14"))),
+  langCxx20OrLater()));
+
   EXPECT_TRUE(matches(
   Code,
   traverse(TK_IgnoreUnlessSpelledInSource,
@@ -3635,6 +3689,16 @@
 matchesConditionally(Code, traverse(TK_IgnoreUnlessSpelledInSource, M),
  true, {"-std=c++20"}));
   }
+  {
+auto M = cxxRewrittenBinaryOperator(
+hasLHS(expr(hasParent(cxxRewrittenBinaryOperator(,
+hasRHS(expr(hasParent(cxxRewrittenBinaryOperator();
+EXPECT_FALSE(
+matchesConditionally(Code, traverse(TK_AsIs, M), true, {"-std=c++20"}));
+EXPECT_TRUE(
+matchesConditionally(Code, traverse(TK_IgnoreUnlessSpelledInSource, M),
+ true, {"-std=c++20"}));
+  }
   {
 EXPECT_TRUE(matchesConditionally(
 Code,
Index: clang/lib/AST/ParentMapContext.cpp
===
--- clang/lib/AST/ParentMapContext.cpp
+++ clang/lib/AST/ParentMapContext.cpp
@@ -49,7 +49,17 @@
   return N;
 }
 
+template 
+std::tuple
+matchParents(const DynTypedNodeList ,
+ ParentMapContext::ParentMap *ParentMap);
+
+template  struct MatchParents;
+
 class ParentMapContext::ParentMap {
+
+  template  friend struct ::MatchParents;
+
   /// Contains parents of a node.
   using ParentVector = llvm::SmallVector;
 
@@ -117,11 +127,75 @@
 if (Node.getNodeKind().hasPointerIdentity()) {
   auto ParentList =
   getDynNodeFromMap(Node.getMemoizationData(), PointerParents);
-