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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Hi Aleksei,

I am OK with this, I just have a little concern about friend declarations. Since https://reviews.llvm.org/D32947 ( [ASTImporter] FriendDecl importing improvements ) records' structural equivalency depends on the order of their friend declarations.
Anyway, I think friend related issues can be addressed in a separate patch.

What is a bit more concerning is that `*ToField` can be null and we should handle that.
This could happen if lookup finds the type of the field, but for some reason (e.g. there is an error in structural eq, or the redecl chain is incorrect) it turns out to be non-equivalent with the type of the FromField:

  for (auto *FoundDecl : FoundDecls) {
    if (auto *FoundField = dyn_cast<FieldDecl>(FoundDecl)) {
      // For anonymous fields, match up by index.
      if (!Name && getFieldIndex(D) != getFieldIndex(FoundField))
        continue;
  
      if (Importer.IsStructurallyEquivalent(D->getType(),
                                            FoundField->getType())) {
        Importer.Imported(D, FoundField);
        return FoundField;
      }
  
      Importer.ToDiag(Loc, diag::err_odr_field_type_inconsistent)
        << Name << D->getType() << FoundField->getType();
      Importer.ToDiag(FoundField->getLocation(), diag::note_odr_value_here)
        << FoundField->getType();
      return nullptr;
    }
  }





================
Comment at: lib/AST/ASTImporter.cpp:1025
+  // type import can depend on them.
+  const RecordDecl *FromRD = dyn_cast<RecordDecl>(FromDC);
+  if (!FromRD)
----------------
Could use `auto` here, to avoid redundant type specification.


================
Comment at: lib/AST/ASTImporter.cpp:1029
+
+  RecordDecl *ToRD = cast<RecordDecl>(Importer.Import(cast<Decl>(FromDC)));
+
----------------
Can't we just import the `FromRD`, why we need that cast at the end? 
`auto *ToRD = cast<RecordDecl>(Importer.Import(FromRD)));` 


================
Comment at: lib/AST/ASTImporter.cpp:1032
+  for (auto *FromField : FromRD->fields()) {
+    Decl *ToField = Importer.GetAlreadyImportedOrNull(FromField);
+    assert(ToRD == ToField->getDeclContext() && ToRD->containsDecl(ToField));
----------------
I think `ToField` can be a nullptr, and if so, then `ToField->getDeclContext()` is UB.
Same could happen at line 1040.
Perhaps we should have and explicit check about the nullptr value:
`if (!ToField) continue;`



================
Comment at: unittests/AST/ASTImporterTest.cpp:1022
 
+AST_MATCHER_P(RecordDecl, hasFieldOrder, std::vector<StringRef>, Order) {
+  size_t Index = 0;
----------------
The `hasFieldOrder` matcher is already existing.


================
Comment at: unittests/AST/ASTImporterTest.cpp:1034
+
+TEST(ImportDecl, ImportFieldOrder) {
+  MatchVerifier<Decl> Verifier;
----------------
We already have this test, we just have to enable it.
`DISABLED_CXXRecordDeclFieldOrderShouldNotDependOnImportOrder`


Repository:
  rC Clang

https://reviews.llvm.org/D44100