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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
