martong updated this revision to Diff 199629.
martong marked 13 inline comments as done.
martong added a comment.
- Address Shafik's comments and update assertions
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D44100/new/
https://reviews.llvm.org/D44100
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
@@ -1371,7 +1371,7 @@
}
TEST_P(ASTImporterOptionSpecificTestBase,
- DISABLED_CXXRecordDeclFieldOrderShouldNotDependOnImportOrder) {
+ CXXRecordDeclFieldOrderShouldNotDependOnImportOrder) {
Decl *From, *To;
std::tie(From, To) = getImportedDecl(
// The original recursive algorithm of ASTImporter first imports 'c' then
@@ -4626,5 +4626,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
Index: clang/lib/AST/ASTImporter.cpp
===================================================================
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1632,13 +1632,77 @@
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());
+ }
+ }
+
+ if (!FromRD)
+ return Error::success();
+
+ // We reorder declarations in RecordDecls because they may have another order
+ // in the "to" context than they have in the "from" context. This may happen
+ // e.g when we import a class like this:
+ // struct declToImport {
+ // int a = c + b;
+ // int b = 1;
+ // int c = 2;
+ // };
+ // During the import of `a` we import first the dependencies in sequence,
+ // thus the order would be `c`, `b`, `a`. We will get the normal order by
+ // first removing the already imported members and then adding them in the
+ // order as they apper in the "from" context.
+ //
+ // Keeping field order is vital because it determines structure layout.
+ //
+ // Here and below, we cannot call field_begin() method and its callers
+ // on ToRD if it has an external storage. Calling field_begin() will
+ // automatically load all the fields by calling
+ // LoadFieldsFromExternalStorage().
+ 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)) {
+ assert(D && "DC has a contained decl which is null?");
+ Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+ assert(ToD && "We already imported all the contained decls of the DC");
+ assert(ToRD == ToD->getLexicalDeclContext() && ToRD->containsDecl(ToD));
+ ToRD->removeDecl(ToD);
+ }
+ }
+
+ // We removed only those fields which we have imported from the FromDC. We
+ // did not remove all the decls of ToRD. And we don't want to remove any
+ // fields which may be loaded from an external storage. The reason is that
+ // field_empty would call field_begin which would call
+ // LoadFieldsFromExternalStorage which in turn would call
+ // ASTImporter::Import. This is because the ExternalASTSource interface in
+ // LLDB is implemented by the means of the ASTImporter. However, calling an
+ // import at this point would result in an uncontrolled import, we must avoid
+ // that.
+ if (!ToRD->hasExternalLexicalStorage())
+ assert(ToRD->field_empty());
+
+ for (auto *D : FromRD->decls()) {
+ if (isa<FieldDecl>(D) || isa<FriendDecl>(D)) {
+ Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+ assert(ToD);
+ assert(ToRD == ToD->getLexicalDeclContext());
+ assert(ToRD->hasExternalLexicalStorage() || !ToRD->containsDecl(ToD));
+ ToRD->addDeclInternal(ToD);
+ }
}
return Error::success();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
