[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<VarDecl, DeclStmt, CXXForRangeStmt>(
+              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<const DeclStmt *>(AncestorNodes)) {
+            return std::get<DynTypedNodeList>(AncestorNodes);
+          }
----------------



================
Comment at: clang/lib/AST/ParentMapContext.cpp:185
+          auto AncestorNodes =
+              matchParents<CXXMethodDecl, CXXRecordDecl, LambdaExpr>(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<bool>(AncestorNodes)) {
+            return std::get<DynTypedNodeList>(AncestorNodes);
+          }
----------------



================
Comment at: clang/lib/AST/ParentMapContext.cpp:195-197
+          if (std::get<bool>(AncestorNodes)) {
+            return std::get<DynTypedNodeList>(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