r345545 - [ASTImporter] Reorder fields after structure import is finished

[Aleksei Sidorin via cfe-commits]

Author: a.sidorin
Date: Mon Oct 29 14:46:18 2018
New Revision: 345545

URL: http://llvm.org/viewvc/llvm-project?rev=345545&view=rev
Log:
[ASTImporter] Reorder fields after structure import is finished

There are multiple reasons why field structures can be imported
in wrong order. The simplest is the ability of field initializers
and method bodies to refer fields not in order they are listed in.
Unfortunately, there is no clean solution for that currently
so I'm leaving a FIXME.

Differential Revision: https://reviews.llvm.org/D44100


Modified:
    cfe/trunk/lib/AST/ASTImporter.cpp
    cfe/trunk/unittests/AST/ASTImporterTest.cpp

Modified: cfe/trunk/lib/AST/ASTImporter.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTImporter.cpp?rev=345545&r1=345544&r2=345545&view=diff
==============================================================================
--- cfe/trunk/lib/AST/ASTImporter.cpp (original)
+++ cfe/trunk/lib/AST/ASTImporter.cpp Mon Oct 29 14:46:18 2018
@@ -1658,13 +1658,53 @@ ASTNodeImporter::ImportDeclContext(DeclC
     auto ToDCOrErr = Importer.ImportContext(FromDC);
     return ToDCOrErr.takeError();
   }
-  llvm::SmallVector<Decl *, 8> ImportedDecls;
+
+  const auto *FromRD = dyn_cast<RecordDecl>(FromDC);
   for (auto *From : FromDC->decls()) {
     ExpectedDecl ImportedOrErr = import(From);
-    if (!ImportedOrErr)
+    if (!ImportedOrErr) {
+      // For RecordDecls, failed import of a field will break the layout of the
+      // structure. Handle it as an error.
+      if (FromRD)
+        return ImportedOrErr.takeError();
       // Ignore the error, continue with next Decl.
       // FIXME: Handle this case somehow better.
-      consumeError(ImportedOrErr.takeError());
+      else
+        consumeError(ImportedOrErr.takeError());
+    }
+  }
+
+  // Reorder declarations in RecordDecls because they may have another
+  // order. Keeping field order is vitable because it determines structure
+  // layout.
+  // FIXME: This is an ugly fix. Unfortunately, I cannot come with better
+  // solution for this issue. We cannot defer expression import here because
+  // type import can depend on them.
+  if (!FromRD)
+    return Error::success();
+
+  auto ImportedDC = import(cast<Decl>(FromDC));
+  assert(ImportedDC);
+  auto *ToRD = cast<RecordDecl>(*ImportedDC);
+
+  for (auto *D : FromRD->decls()) {
+    if (isa<FieldDecl>(D) || isa<FriendDecl>(D)) {
+      Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+      assert(ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD));
+      ToRD->removeDecl(ToD);
+    }
+  }
+
+  assert(ToRD->field_empty());
+
+  for (auto *D : FromRD->decls()) {
+    if (isa<FieldDecl>(D) || isa<FriendDecl>(D)) {
+      Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+      assert(ToRD == ToD->getDeclContext());
+      assert(ToRD == ToD->getLexicalDeclContext());
+      assert(!ToRD->containsDecl(ToD));
+      ToRD->addDeclInternal(ToD);
+    }
   }
 
   return Error::success();

Modified: cfe/trunk/unittests/AST/ASTImporterTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/ASTImporterTest.cpp?rev=345545&r1=345544&r2=345545&view=diff
==============================================================================
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp (original)
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp Mon Oct 29 14:46:18 2018
@@ -1457,7 +1457,7 @@ TEST_P(ASTImporterTestBase, CXXRecordDec
 }
 
 TEST_P(ASTImporterTestBase,
-       DISABLED_CXXRecordDeclFieldOrderShouldNotDependOnImportOrder) {
+       CXXRecordDeclFieldOrderShouldNotDependOnImportOrder) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
       // The original recursive algorithm of ASTImporter first imports 'c' then
@@ -3767,5 +3767,16 @@ INSTANTIATE_TEST_CASE_P(ParameterizedTes
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportVariables,
                         DefaultTestValuesForRunOptions, );
 
+TEST_P(ImportDecl, ImportFieldOrder) {
+  MatchVerifier<Decl> Verifier;
+  testImport("struct declToImport {"
+             "  int b = a + 2;"
+             "  int a = 5;"
+             "};",
+             Lang_CXX11, "", Lang_CXX11, Verifier,
+             recordDecl(hasFieldOrder({"b", "a"})));
+}
+
+
 } // end namespace ast_matchers
 } // end namespace clang