[PATCH] D156093: [ASTImporter] Add extra sorting round for keeping lexical order of all imported decls within record

[Ding Fei via Phabricator via cfe-commits]

danix800 created this revision.
danix800 added a project: clang.
Herald added a subscriber: martong.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: shafik.
Herald added a project: All.
danix800 requested review of this revision.
Herald added a subscriber: cfe-commits.

Revision https://reviews.llvm.org/D154764 is still not complete. An extra 
sorting is needed
for keeping lexical order of all imported decls within a record. The final 
algorithm could be
summarized as the following:

1. Import all fields that'll be part of the layout;
2. Sort these fields to ensure correct layout;
3. Import everything else;
4. Final sort for correct lexical order.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156093

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
@@ -1462,10 +1462,10 @@
       MatchVerifier<Decl>{}.match(To->getTranslationUnitDecl(), Pattern));
 }
 
-AST_MATCHER_P(RecordDecl, hasFieldOrder, std::vector<StringRef>, Order) {
+AST_MATCHER_P(RecordDecl, hasLexicalOrder, std::vector<StringRef>, Order) {
   size_t Index = 0;
   for (Decl *D : Node.decls()) {
-    if (isa<FieldDecl>(D) || isa<IndirectFieldDecl>(D)) {
+    if (isa<FieldDecl>(D) || isa<IndirectFieldDecl>(D) || !D->isImplicit()) {
       auto *ND = cast<NamedDecl>(D);
       if (Index == Order.size())
         return false;
@@ -1513,8 +1513,8 @@
                       Lang_CXX11, "", Lang_CXX11);
 
   MatchVerifier<Decl> Verifier;
-  ASSERT_TRUE(Verifier.match(From, cxxRecordDecl(hasFieldOrder({"a", "b"}))));
-  EXPECT_TRUE(Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b"}))));
+  ASSERT_TRUE(Verifier.match(From, cxxRecordDecl(hasLexicalOrder({"a", "b"}))));
+  EXPECT_TRUE(Verifier.match(To, cxxRecordDecl(hasLexicalOrder({"a", "b"}))));
 }
 
 TEST_P(ASTImporterOptionSpecificTestBase,
@@ -1532,11 +1532,30 @@
       )s",
       Lang_CXX11, "", Lang_CXX11);
 
+  auto Pattern = cxxRecordDecl(hasLexicalOrder({"a", "b", "c"}));
   MatchVerifier<Decl> Verifier;
-  ASSERT_TRUE(
-      Verifier.match(From, cxxRecordDecl(hasFieldOrder({"a", "b", "c"}))));
-  EXPECT_TRUE(
-      Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b", "c"}))));
+  ASSERT_TRUE(Verifier.match(From, Pattern));
+  EXPECT_TRUE(Verifier.match(To, Pattern));
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase, CXXRecordDeclCorrectLexicalOrder) {
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+      R"s(
+      struct declToImport {
+          int a = c;
+          void g() { h(); }
+          int b = 1;
+          void h();
+          int c = 2;
+      };
+      )s",
+      Lang_CXX11, "", Lang_CXX11);
+
+  auto Pattern = cxxRecordDecl(hasLexicalOrder({"a", "g", "b", "h", "c"}));
+  MatchVerifier<Decl> Verifier;
+  ASSERT_TRUE(Verifier.match(From, Pattern));
+  EXPECT_TRUE(Verifier.match(To, Pattern));
 }
 
 TEST_P(ASTImporterOptionSpecificTestBase,
@@ -1557,11 +1576,17 @@
       )s",
       Lang_CXX11, "", Lang_CXX11);
 
+  auto Pattern = cxxRecordDecl(hasLexicalOrder({
+    "a",
+    "" /*anony union*/,
+    "" /* implicit field */,
+    "b" /* indirect */,
+    "c" /* indirect */,
+    "d"
+  }));
   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"}))));
+  ASSERT_TRUE(Verifier.match(From, Pattern));
+  EXPECT_TRUE(Verifier.match(To, Pattern));
 }
 
 TEST_P(ASTImporterOptionSpecificTestBase, ShouldImportImplicitCXXRecordDecl) {
@@ -3007,7 +3032,7 @@
              "  int a = 5;"
              "};",
              Lang_CXX11, "", Lang_CXX11, Verifier,
-             recordDecl(hasFieldOrder({"b", "a"})));
+             recordDecl(hasLexicalOrder({"b", "a"})));
 }
 
 const internal::VariadicDynCastAllOfMatcher<Expr, DependentScopeDeclRefExpr>
Index: clang/lib/AST/ASTImporter.cpp
===================================================================
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1942,6 +1942,16 @@
                                                 ImportedOrErr.takeError());
   }
 
+  // Final round to re-order everything
+  for (auto *D : FromDC->decls()) {
+    assert(D && "DC contains a null decl");
+    if (Decl *ToD = Importer.GetAlreadyImportedOrNull(D); ToD &&
+        ToDC == ToD->getLexicalDeclContext() && ToDC->containsDecl(ToD)) {
+      ToDC->removeDecl(ToD);
+      ToDC->addDeclInternal(ToD);
+    }
+  }
+
   return ChildErrors;
 }
 

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits