a_sidorin added a comment.
Hello Balasz,
I have some comments inline.
================
Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1579
+ D2 = D2->getCanonicalDecl();
+ std::pair<Decl *, Decl *> P = std::make_pair(D1, D2);
+
----------------
`std::pair<Decl *, Decl *> P{D1, D2}`?
================
Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:1888
- Decl *D2 = TentativeEquivalences[D1];
- assert(D2 && "Unrecorded tentative equivalence?");
+ Decl *D1 = P.first;
+ Decl *D2 = P.second;
----------------
I would prefer std::tie instead: `std::tie(D1, D2) = P;`. But it is a matter of
taste.
================
Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1290
+ bool isInNonEqCache(std::tuple<NodeType *, NodeType *> D) {
+ return NonEquivalentDecls.find(std::make_pair(get<0>(D), get<1>(D))) !=
+ NonEquivalentDecls.end();
----------------
Can we use std::pair instead of std::tuple in this class? The size of tuple is
known to be 2 and it will help to avoid such conversions.
================
Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1291
+ return NonEquivalentDecls.find(std::make_pair(get<0>(D), get<1>(D))) !=
+ NonEquivalentDecls.end();
+ }
----------------
`return NonEquivalentDecls.count({get<0>(D), get<1>(D)});`?
================
Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1314
+ bool Eq = Ctx.IsEquivalent(get<0>(X), get<1>(X));
+ EXPECT_FALSE(Eq);
+
----------------
It seems to me that the assertion will become a bit easier to read without an
intermediate variable. What do you think?
================
Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1357
+
+ EXPECT_FALSE(isInNonEqCache(C));
+ EXPECT_FALSE(isInNonEqCache(findDeclPair<CXXRecordDecl>(
----------------
The declarations of C are not equivalent, but they are not placed into the
non-equivalence cache. This looks strange to me, could you please explain this
behavior?
================
Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:1399
+ TU, cxxRecordDecl(hasName("A"), unless(isImplicit())))));
+ EXPECT_FALSE(isInNonEqCache(
+ findDeclPair<FunctionDecl>(TU, functionDecl(hasName("x")))));
----------------
We don't have any tests where isInNonEqCache() can return true. Is it expected?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66538/new/
https://reviews.llvm.org/D66538
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
