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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
