martong added inline comments.
================
Comment at: clang/lib/AST/ASTImporter.cpp:2021
+ // fields imported) at that time without multiple AST import passes.
+ To->setCompleteDefinition(true);
// Complete the definition even if error is returned.
----------------
balazske wrote:
> balazske wrote:
> > shafik wrote:
> > > So `DefinitionCompleterScopeExit` will run
> > > `To->setCompleteDefinition(false);` after this function exits but this
> > > will be in effect during the import the base classes. I don't see how the
> > > tests you added hit that code.
> > Should the test `CompleteRecordBeforeImporting` not do the import in
> > minimal mode? The comment says minimal but in `ASTImporter` minimal mode is
> > not set. The test will fail because this added line. But I think it should
> > work to call the `To->setCompleteDefinition` here only in non-minimal mode.
> And remove the later added line 2088 `To->setCompleteDefinition(false);`.
> Should the test CompleteRecordBeforeImporting not do the import in minimal
> mode?
Yes, that should, however, I can confirm it probably does not (see the `false`
arg below):
```
Importer.reset(Creator(ToAST->getASTContext(), ToAST->getFileManager(),
Unit->getASTContext(), Unit->getFileManager(), false,
SharedState));
```
So, first we should fix the test case `CompleteRecordBeforeImporting` to set up
the ASTImporter in minimal mode.
Then, we should call the To->setCompleteDefinition here only in non-minimal
mode as you suggests. Once again, an ugly branching because of the "minimal"
mode, we should really get rid of that (and hope that Raphael patch evolves in
D101950).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116155/new/
https://reviews.llvm.org/D116155
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
