[PATCH] D116155: [clang][AST][ASTImporter] Set record to complete during import of its members.

[Gabor Marton via Phabricator via cfe-commits]

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