[PATCH] D75740: [ASTImporter] Corrected import of repeated friend declarations.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hello Balazs,
This look almost good to me except some comments inline.



================
Comment at: clang/lib/AST/ASTImporter.cpp:3635
 
+static std::tuple<unsigned int, unsigned int>
+getFriendCountAndPosition(FriendDecl *FD) {
----------------
It's better to turn the tuple into a named struct (`FriendCountAndPosition`?) to make the code more readable.


================
Comment at: clang/lib/AST/ASTImporter.cpp:3636
+static std::tuple<unsigned int, unsigned int>
+getFriendCountAndPosition(FriendDecl *FD) {
+  unsigned int FriendCount = 0;
----------------
`const FriendDecl *FD`?


================
Comment at: clang/lib/AST/ASTImporter.cpp:3639
+  llvm::Optional<unsigned int> FriendPosition;
+  auto *RD = cast<CXXRecordDecl>(FD->getLexicalDeclContext());
+  if (FD->getFriendType()) {
----------------
const?


================
Comment at: clang/lib/AST/ASTImporter.cpp:3640
+  auto *RD = cast<CXXRecordDecl>(FD->getLexicalDeclContext());
+  if (FD->getFriendType()) {
+    QualType TypeOfFriend = FD->getFriendType()->getType().getCanonicalType();
----------------
```if (const TypeSourceInfo *FriendType = FD->getFriendType()) {
    QualType TypeOfFriend = FriendType->getType().getCanonicalType();
...```
?


================
Comment at: clang/lib/AST/ASTImporter.cpp:3699
+
+  assert(ImportedEquivalentFriends.size() <= std::get<0>(CountAndPosition) &&
+         "Class with non-matching friends is imported, ODR check wrong?");
----------------
Why not strictly equal?


================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:4009
+TEST_P(ImportFriendClasses, ImportOfRepeatedFriendType) {
+  auto Code =
+      R"(
----------------
clang-tidy wants this to be `const auto* Code`.


================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:4034
+TEST_P(ImportFriendClasses, ImportOfRepeatedFriendDecl) {
+  auto Code =
+      R"(
----------------
clang-tidy wants this to be `const auto* Code`.


================
Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:801
+TEST_F(StructuralEquivalenceRecordTest, SameFriendMultipleTimes) {
+  auto t = makeNamedDecls("struct foo{ friend class X; };",
+                          "struct foo{ friend class X; friend class X; };",
----------------
We need to place spaces after `foo`.


================
Comment at: clang/unittests/AST/StructuralEquivalenceTest.cpp:811
+                          Lang_CXX);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
----------------
Could you please add a positive test with two `struct foo{ friend class X; friend class Y; };",` structures as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75740