[PATCH] D66866: [ASTImporter] At import of records re-order indirect fields too.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL370621: [ASTImporter] At import of records re-order indirect
fields too. (authored by balazske, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D66866?vs=217816&id=218294#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66866/new/
https://reviews.llvm.org/D66866
Files:
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/unittests/AST/ASTImporterTest.cpp
Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -1702,7 +1702,7 @@
// Remove all declarations, which may be in wrong order in the
// lexical DeclContext and then add them in the proper order.
for (auto *D : FromRD->decls()) {
-if (isa(D) || isa(D)) {
+if (isa(D) || isa(D) || isa(D)) {
assert(D && "DC contains a null decl");
Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
// Remove only the decls which we successfully imported.
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -1421,12 +1421,15 @@
AST_MATCHER_P(RecordDecl, hasFieldOrder, std::vector, Order) {
size_t Index = 0;
- for (FieldDecl *Field : Node.fields()) {
-if (Index == Order.size())
- return false;
-if (Field->getName() != Order[Index])
- return false;
-++Index;
+ for (Decl *D : Node.decls()) {
+if (isa(D) || isa(D)) {
+ auto *ND = cast(D);
+ if (Index == Order.size())
+return false;
+ if (ND->getName() != Order[Index])
+return false;
+ ++Index;
+}
}
return Index == Order.size();
}
@@ -1493,6 +1496,31 @@
Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b", "c"};
}
+TEST_P(ASTImporterOptionSpecificTestBase,
+ CXXRecordDeclFieldAndIndirectFieldOrder) {
+ Decl *From, *To;
+ std::tie(From, To) = getImportedDecl(
+ // First field is "a", then the field for unnamed union, then "b" and "c"
+ // from it (indirect fields), then "d".
+ R"s(
+ struct declToImport {
+int a = d;
+union {
+ int b;
+ int c;
+};
+int d;
+ };
+ )s",
+ Lang_CXX11, "", Lang_CXX11);
+
+ MatchVerifier Verifier;
+ ASSERT_TRUE(Verifier.match(
+ From, cxxRecordDecl(hasFieldOrder({"a", "", "b", "c", "d"};
+ EXPECT_TRUE(Verifier.match(
+ To, cxxRecordDecl(hasFieldOrder({"a", "", "b", "c", "d"};
+}
+
TEST_P(ASTImporterOptionSpecificTestBase, ShouldImportImplicitCXXRecordDecl) {
Decl *From, *To;
std::tie(From, To) = getImportedDecl(
Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -1702,7 +1702,7 @@
// Remove all declarations, which may be in wrong order in the
// lexical DeclContext and then add them in the proper order.
for (auto *D : FromRD->decls()) {
-if (isa(D) || isa(D)) {
+if (isa(D) || isa(D) || isa(D)) {
assert(D && "DC contains a null decl");
Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
// Remove only the decls which we successfully imported.
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -1421,12 +1421,15 @@
AST_MATCHER_P(RecordDecl, hasFieldOrder, std::vector, Order) {
size_t Index = 0;
- for (FieldDecl *Field : Node.fields()) {
-if (Index == Order.size())
- return false;
-if (Field->getName() != Order[Index])
- return false;
-++Index;
+ for (Decl *D : Node.decls()) {
+if (isa(D) || isa(D)) {
+ auto *ND = cast(D);
+ if (Index == Order.size())
+return false;
+ if (ND->getName() != Order[Index])
+return false;
+ ++Index;
+}
}
return Index == Order.size();
}
@@ -1493,6 +1496,31 @@
Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b", "c"};
}
+TEST_P(ASTImporterOptionSpecificTestBase,
+ CXXRecordDeclFieldAndIndirectFieldOrder) {
+ Decl *From, *To;
+ std::tie(From, To) = getImportedDecl(
+ // First field is "a", then the field for unnamed union, then "b" and "c"
+ // from it (indirect fields), then "d".
+ R"s(
+ struct declToImport {
+int a = d;
+union {
+ int b;
+ int c;
+};
+int d;
+ };
+ )s",
+ Lang_CXX11, "", Lang_CXX11);
+
+ MatchVerifier Verifier;
+ ASSERT_TRUE(Verifier.match(
+ From, cxxRecordDecl(hasField
[PATCH] D66866: [ASTImporter] At import of records re-order indirect fields too.
a_sidorin accepted this revision. a_sidorin added a comment. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66866/new/ https://reviews.llvm.org/D66866 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66866: [ASTImporter] At import of records re-order indirect fields too.
martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66866/new/ https://reviews.llvm.org/D66866 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66866: [ASTImporter] At import of records re-order indirect fields too.
balazske updated this revision to Diff 217816.
balazske added a comment.
- Simplified field matcher in tests.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66866/new/
https://reviews.llvm.org/D66866
Files:
clang/lib/AST/ASTImporter.cpp
clang/unittests/AST/ASTImporterTest.cpp
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -1421,12 +1421,15 @@
AST_MATCHER_P(RecordDecl, hasFieldOrder, std::vector, Order) {
size_t Index = 0;
- for (FieldDecl *Field : Node.fields()) {
-if (Index == Order.size())
- return false;
-if (Field->getName() != Order[Index])
- return false;
-++Index;
+ for (Decl *D : Node.decls()) {
+if (isa(D) || isa(D)) {
+ auto *ND = cast(D);
+ if (Index == Order.size())
+return false;
+ if (ND->getName() != Order[Index])
+return false;
+ ++Index;
+}
}
return Index == Order.size();
}
@@ -1493,6 +1496,31 @@
Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b", "c"};
}
+TEST_P(ASTImporterOptionSpecificTestBase,
+ CXXRecordDeclFieldAndIndirectFieldOrder) {
+ Decl *From, *To;
+ std::tie(From, To) = getImportedDecl(
+ // First field is "a", then the field for unnamed union, then "b" and "c"
+ // from it (indirect fields), then "d".
+ R"s(
+ struct declToImport {
+int a = d;
+union {
+ int b;
+ int c;
+};
+int d;
+ };
+ )s",
+ Lang_CXX11, "", Lang_CXX11);
+
+ MatchVerifier Verifier;
+ ASSERT_TRUE(Verifier.match(
+ From, cxxRecordDecl(hasFieldOrder({"a", "", "b", "c", "d"};
+ EXPECT_TRUE(Verifier.match(
+ To, cxxRecordDecl(hasFieldOrder({"a", "", "b", "c", "d"};
+}
+
TEST_P(ASTImporterOptionSpecificTestBase, ShouldImportImplicitCXXRecordDecl) {
Decl *From, *To;
std::tie(From, To) = getImportedDecl(
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1701,7 +1701,7 @@
// Remove all declarations, which may be in wrong order in the
// lexical DeclContext and then add them in the proper order.
for (auto *D : FromRD->decls()) {
-if (isa(D) || isa(D)) {
+if (isa(D) || isa(D) || isa(D)) {
assert(D && "DC contains a null decl");
Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
// Remove only the decls which we successfully imported.
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -1421,12 +1421,15 @@
AST_MATCHER_P(RecordDecl, hasFieldOrder, std::vector, Order) {
size_t Index = 0;
- for (FieldDecl *Field : Node.fields()) {
-if (Index == Order.size())
- return false;
-if (Field->getName() != Order[Index])
- return false;
-++Index;
+ for (Decl *D : Node.decls()) {
+if (isa(D) || isa(D)) {
+ auto *ND = cast(D);
+ if (Index == Order.size())
+return false;
+ if (ND->getName() != Order[Index])
+return false;
+ ++Index;
+}
}
return Index == Order.size();
}
@@ -1493,6 +1496,31 @@
Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b", "c"};
}
+TEST_P(ASTImporterOptionSpecificTestBase,
+ CXXRecordDeclFieldAndIndirectFieldOrder) {
+ Decl *From, *To;
+ std::tie(From, To) = getImportedDecl(
+ // First field is "a", then the field for unnamed union, then "b" and "c"
+ // from it (indirect fields), then "d".
+ R"s(
+ struct declToImport {
+int a = d;
+union {
+ int b;
+ int c;
+};
+int d;
+ };
+ )s",
+ Lang_CXX11, "", Lang_CXX11);
+
+ MatchVerifier Verifier;
+ ASSERT_TRUE(Verifier.match(
+ From, cxxRecordDecl(hasFieldOrder({"a", "", "b", "c", "d"};
+ EXPECT_TRUE(Verifier.match(
+ To, cxxRecordDecl(hasFieldOrder({"a", "", "b", "c", "d"};
+}
+
TEST_P(ASTImporterOptionSpecificTestBase, ShouldImportImplicitCXXRecordDecl) {
Decl *From, *To;
std::tie(From, To) = getImportedDecl(
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1701,7 +1701,7 @@
// Remove all declarations, which may be in wrong order in the
// lexical DeclContext and then add them in the proper order.
for (auto *D : FromRD->decls()) {
-if (isa(D) || isa(D)) {
+if (isa(D) || isa(D) || isa(D)) {
assert(D && "DC contains a null decl");
Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
// Remove only the decls which we successfully imported.
___
[PATCH] D66866: [ASTImporter] At import of records re-order indirect fields too.
martong added a comment.
Looks good, I just have a comment about the matcher.
Comment at: clang/unittests/AST/ASTImporterTest.cpp:1434
+AST_MATCHER_P(RecordDecl, hasFieldIndirectFieldOrder, std::vector,
+ Order) {
This name sounds strange for me, perhaps `hasAnyKindOfFieldOrder` ?
Also, this matcher I think supersedes the `hasFieldOrder`, I mean in the tests
where we use `hasFieldOrder` we could use this new matcher too, can't we? In
that case, however, we can delete the old implementation.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66866/new/
https://reviews.llvm.org/D66866
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66866: [ASTImporter] At import of records re-order indirect fields too.
balazske created this revision.
Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp.
Herald added a reviewer: martong.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: shafik.
Herald added a project: clang.
Correct order of fields and indirect fields in imported RecordDecl
is needed for correct work of record layout calculations.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D66866
Files:
clang/lib/AST/ASTImporter.cpp
clang/unittests/AST/ASTImporterTest.cpp
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -1431,6 +1431,23 @@
return Index == Order.size();
}
+AST_MATCHER_P(RecordDecl, hasFieldIndirectFieldOrder, std::vector,
+ Order) {
+ size_t Index = 0;
+ for (Decl *D : Node.decls()) {
+if (isa(D) || isa(D)) {
+ if (auto *ND = cast(D)) {
+if (Index == Order.size())
+ return false;
+if (ND->getName() != Order[Index])
+ return false;
+++Index;
+ }
+}
+ }
+ return Index == Order.size();
+}
+
TEST_P(ASTImporterOptionSpecificTestBase,
TUshouldContainClassTemplateSpecializationOfExplicitInstantiation) {
Decl *From, *To;
@@ -1493,6 +1510,31 @@
Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b", "c"};
}
+TEST_P(ASTImporterOptionSpecificTestBase,
+ CXXRecordDeclFieldAndIndirectFieldOrder) {
+ Decl *From, *To;
+ std::tie(From, To) = getImportedDecl(
+ // First field is "a", then the field for unnamed union, then "b" and "c"
+ // from it, then "d".
+ R"s(
+ struct declToImport {
+int a = d;
+union {
+ int b;
+ int c;
+};
+int d;
+ };
+ )s",
+ Lang_CXX11, "", Lang_CXX11);
+
+ MatchVerifier Verifier;
+ ASSERT_TRUE(Verifier.match(From, cxxRecordDecl(hasFieldIndirectFieldOrder(
+ {"a", "", "b", "c", "d"};
+ EXPECT_TRUE(Verifier.match(
+ To, cxxRecordDecl(hasFieldIndirectFieldOrder({"a", "", "b", "c",
"d"};
+}
+
TEST_P(ASTImporterOptionSpecificTestBase, ShouldImportImplicitCXXRecordDecl) {
Decl *From, *To;
std::tie(From, To) = getImportedDecl(
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1701,7 +1701,7 @@
// Remove all declarations, which may be in wrong order in the
// lexical DeclContext and then add them in the proper order.
for (auto *D : FromRD->decls()) {
-if (isa(D) || isa(D)) {
+if (isa(D) || isa(D) || isa(D)) {
assert(D && "DC contains a null decl");
Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
// Remove only the decls which we successfully imported.
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -1431,6 +1431,23 @@
return Index == Order.size();
}
+AST_MATCHER_P(RecordDecl, hasFieldIndirectFieldOrder, std::vector,
+ Order) {
+ size_t Index = 0;
+ for (Decl *D : Node.decls()) {
+if (isa(D) || isa(D)) {
+ if (auto *ND = cast(D)) {
+if (Index == Order.size())
+ return false;
+if (ND->getName() != Order[Index])
+ return false;
+++Index;
+ }
+}
+ }
+ return Index == Order.size();
+}
+
TEST_P(ASTImporterOptionSpecificTestBase,
TUshouldContainClassTemplateSpecializationOfExplicitInstantiation) {
Decl *From, *To;
@@ -1493,6 +1510,31 @@
Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b", "c"};
}
+TEST_P(ASTImporterOptionSpecificTestBase,
+ CXXRecordDeclFieldAndIndirectFieldOrder) {
+ Decl *From, *To;
+ std::tie(From, To) = getImportedDecl(
+ // First field is "a", then the field for unnamed union, then "b" and "c"
+ // from it, then "d".
+ R"s(
+ struct declToImport {
+int a = d;
+union {
+ int b;
+ int c;
+};
+int d;
+ };
+ )s",
+ Lang_CXX11, "", Lang_CXX11);
+
+ MatchVerifier Verifier;
+ ASSERT_TRUE(Verifier.match(From, cxxRecordDecl(hasFieldIndirectFieldOrder(
+ {"a", "", "b", "c", "d"};
+ EXPECT_TRUE(Verifier.match(
+ To, cxxRecordDecl(hasFieldIndirectFieldOrder({"a", "", "b", "c", "d"};
+}
+
TEST_P(ASTImporterOptionSpecificTestBase, ShouldImportImplicitCXXRecordDecl) {
Decl *From, *To;
std::tie(From, To) = getImportedDecl(
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1701,7 +1701,7 @@
// Remove all declarations, whi
