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

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Done with first round.



================
Comment at: lib/AST/ASTImporter.cpp:1643
+    if (!ImportedOrErr) {
+      // For RecordDecls, failed import of a field will break the layout of the
+      // structure. Handle it as an error.
----------------
What cases are failed import of a field ok? Is that because we will get the field later on by another path?


================
Comment at: lib/AST/ASTImporter.cpp:1655
+  // Reorder declarations in RecordDecls because they may have another
+  // order. Keeping field order is vitable because it determines structure
+  // layout.
----------------
vitable?


================
Comment at: lib/AST/ASTImporter.cpp:1663
+
+
+  // NOTE: Here and below, we cannot call field_begin() method and its callers
----------------
Maybe a comment here explaining the purpose of the loops below. IIUC removing fields since they may have been imported in the wrong order and then adding them back in what should be the correct order now.


================
Comment at: lib/AST/ASTImporter.cpp:1674
+      Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+      assert(ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD));
+      ToRD->removeDecl(ToD);
----------------
Are we sure `ToD` is never a `nullptr` b/c you are unconditionally derefenecing it here.


================
Comment at: lib/AST/ASTImporter.cpp:1679
+
+  if (!ToRD->hasExternalLexicalStorage())
+    assert(ToRD->field_empty());
----------------
Can you add a comment explaining why if the Decl has ExternalLexicalStorage the fields might not be empty even though we just removed them?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44100/new/

https://reviews.llvm.org/D44100