[PATCH] D66538: [AST] AST structural equivalence to work internally with pairs.

[Aleksei Sidorin via Phabricator via cfe-commits]

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