[ASTImporter] At import of records re-order indirect fields too.

[balazske]

Summary

Correct order of fields and indirect fields in imported RecordDecl
is needed for correct work of record layout calculations.

[martong]

Looks good, I just have a comment about the matcher.

clang/unittests/AST/ASTImporterTest.cpp

1434 ↗

(On Diff #217591)

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.

[balazske]

• Simplified field matcher in tests.

[martong]

LGTM!

[a_sidorin]

LGTM.

cfe/trunk/lib/AST/ASTImporter.cpp

if (!ToDCOrErr) {
     consumeError(std::move(ChildErrors));
     return ToDCOrErr.takeError();
   }
 
   DeclContext *ToDC = *ToDCOrErr;
   // 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<FieldDecl>(D) || isa<FriendDecl>(D)) {
+    if (isa<FieldDecl>(D) || isa<IndirectFieldDecl>(D) || isa<FriendDecl>(D)) {
       assert(D && "DC contains a null decl");
       Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
       // Remove only the decls which we successfully imported.
       if (ToD) {
         assert(ToDC == ToD->getLexicalDeclContext() && ToDC->containsDecl(ToD));
         // Remove the decl from its wrong place in the linked list.
         ToDC->removeDecl(ToD);
         // Add the decl to the end of the linked list.

cfe/trunk/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterOptionSpecificTestBase,
   ASSERT_TRUE(
       MatchVerifier<Decl>{}.match(From->getTranslationUnitDecl(), Pattern));
   EXPECT_TRUE(
       MatchVerifier<Decl>{}.match(To->getTranslationUnitDecl(), Pattern));
 }
 
 AST_MATCHER_P(RecordDecl, hasFieldOrder, std::vector<StringRef>, Order) {
   size_t Index = 0;
-  for (FieldDecl *Field : Node.fields()) {
+  for (Decl *D : Node.decls()) {
+    if (isa<FieldDecl>(D) || isa<IndirectFieldDecl>(D)) {
+      auto *ND = cast<NamedDecl>(D);
       if (Index == Order.size())
         return false;
-    if (Field->getName() != Order[Index])
+      if (ND->getName() != Order[Index])
         return false;
       ++Index;
     }
+  }
   return Index == Order.size();
 }
 
 TEST_P(ASTImporterOptionSpecificTestBase,
        TUshouldContainClassTemplateSpecializationOfExplicitInstantiation) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
       R"(
TEST_P(ASTImporterOptionSpecificTestBase,
 
   MatchVerifier<Decl> Verifier;
   ASSERT_TRUE(
       Verifier.match(From, cxxRecordDecl(hasFieldOrder({"a", "b", "c"}))));
   EXPECT_TRUE(
       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<Decl> 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(
       R"(
       struct declToImport {
       };
       )",
       Lang_CXX, "", Lang_CXX);