martong added a comment. Looks okay to me (other than the redundant import code that I have a comment about).
> Also this seems to be testable via a Clang unit test, so I think this patch > should have one. Yeah, would be nice to have a Clang unit test. I believe we have the infrastructure for that in place. E.g. in `LLDBLookupTest` we have an lldb specific setup, that could be a good starting point. ================ 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. > 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? ================ Comment at: clang/lib/AST/ASTImporter.cpp:8179 + + if (FromRecord->isCompleteDefinition()) + if (Error Err = ASTNodeImporter(*this).ImportDefinition( ---------------- We could merge this `if` with the `else if` at line 8164, because their body is exactly the same. But to do that, we should move the external storage query and type completion above the enclosing `if` (above line 8162 and just after line 8161). 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
