[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

[Aleksei Sidorin via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC345545: [ASTImporter] Reorder fields after structure import is finished (authored by a.sidorin, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D44100?vs=160477&id=171578#toc

Repository:
  rC Clang

https://reviews.llvm.org/D44100

Files:
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp


Index: lib/AST/ASTImporter.cpp
===================================================================
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1658,13 +1658,53 @@
     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();
Index: unittests/AST/ASTImporterTest.cpp
===================================================================
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1457,7 +1457,7 @@
 }
 
 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(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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D44100.171578.patch
Type: text/x-patch
Size: 3142 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181029/f33ddcb0/attachment.bin>