[Lldb-commits] [PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik marked an inline comment as done.
shafik added inline comments.


================
Comment at: clang/lib/AST/ASTImporter.cpp:8173
+      // If a RecordDecl has base classes they won't be imported and we will
+      // be missing anything that we inherit from those bases.
+      if (FromRecord->hasExternalLexicalStorage() &&
----------------
teemperor wrote:
> I'm not sure how it can be that ASTImporter::CompleteDecl starts but never finishes the decl as it does both things at once unconditionally?
> ```
> lang=c++
>       TD->startDefinition();
>       TD->setCompleteDefinition(true);
> ```
> 
> FWIW, this whole comment could just be `Ask the external source if this is actually a complete record that just hasn't been completed yet` or something like this. I think if we reach this point then it makes a complete sense to ask the external source for more info. The explanation about the base classes seems to be just a smaller detail of the whole picture here.
`TD->setCompleteDefinition(true);` just sets a flag but it does not import the bases, which it can not do since From is not defined. See `ASTNodeImporter::ImportDefinition(RecordDecl *From, RecordDecl *To, ImportDefinitionKind Kind)` which does this after it does `To->startDefinition();`. So we can't really consider the definition finished if we don't do this. 

Do you have a better way of describing this situation?

I think it is important to describe why we don't use `CompleteDecl` since the other cases do.


================
Comment at: clang/lib/AST/ASTImporter.cpp:8173
+      // If a RecordDecl has base classes they won't be imported and we will
+      // be missing anything that we inherit from those bases.
+      if (FromRecord->hasExternalLexicalStorage() &&
----------------
martong wrote:
> shafik wrote:
> > teemperor wrote:
> > > I'm not sure how it can be that ASTImporter::CompleteDecl starts but never finishes the decl as it does both things at once unconditionally?
> > > ```
> > > lang=c++
> > >       TD->startDefinition();
> > >       TD->setCompleteDefinition(true);
> > > ```
> > > 
> > > FWIW, this whole comment could just be `Ask the external source if this is actually a complete record that just hasn't been completed yet` or something like this. I think if we reach this point then it makes a complete sense to ask the external source for more info. The explanation about the base classes seems to be just a smaller detail of the whole picture here.
> > `TD->setCompleteDefinition(true);` just sets a flag but it does not import the bases, which it can not do since From is not defined. See `ASTNodeImporter::ImportDefinition(RecordDecl *From, RecordDecl *To, ImportDefinitionKind Kind)` which does this after it does `To->startDefinition();`. So we can't really consider the definition finished if we don't do this. 
> > 
> > Do you have a better way of describing this situation?
> > 
> > I think it is important to describe why we don't use `CompleteDecl` since the other cases do.
> > Ask the external source if this is actually a complete record that just hasn't been completed yet
> 
> FWIW this seems to be a recurring pattern, so maybe it would be worth to do this not just with `RecordDecl`s but with other kind of decls. E.g. an `EnumDecl` could have an external source and not  completed, couldn't it?
This is definitely more costly, for the `EnumDecl` case I don't see how we could get in a similar situation since we don't have inheritance. I looked over the `EnumDecl` members and I don't see anything that would require it be defined.


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

https://reviews.llvm.org/D78000