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