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

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
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.
----------------
shafik wrote:
> What cases are failed import of a field ok? Is that because we will get the field later on by another path?
The decl context of which we import the contained decls (`FromDC`) may be a namespace, it is not necessarily a RecordDecl.
I guess it would be too drastic if a whole namespace is rendered as erroneous if just one contained decl import failed.


================
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.
----------------
shafik wrote:
> vitable?
My guess is `vital`.


================
Comment at: lib/AST/ASTImporter.cpp:1674
+      Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+      assert(ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD));
+      ToRD->removeDecl(ToD);
----------------
shafik wrote:
> Are we sure `ToD` is never a `nullptr` b/c you are unconditionally derefenecing it here.
Yes, I agree. Added `assert(ToD)` because we had imported all contained decls previously without errors.


================
Comment at: lib/AST/ASTImporter.cpp:1674
+      Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+      assert(ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD));
+      ToRD->removeDecl(ToD);
----------------
martong wrote:
> shafik wrote:
> > Are we sure `ToD` is never a `nullptr` b/c you are unconditionally derefenecing it here.
> Yes, I agree. Added `assert(ToD)` because we had imported all contained decls previously without errors.
`assert(ToRD == ToD->getDeclContext()` is not correct here, because friends semantic decl context may not be the befriending class. Changed to compare against the lexical DC.


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


================
Comment at: lib/AST/ASTImporter.cpp:1686
+      assert(ToD);
+      assert(ToRD == ToD->getDeclContext());
+      assert(ToRD == ToD->getLexicalDeclContext());
----------------
I removed this assert, because friends most usually have a different semantic context than the befriending class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D44100