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