[PATCH] D69933: [ASTImporter] Limit imports of structs

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

I really don't think the ASTImporter should ever manipulate records in the source context (effectively the source context should be considered immutable). It also seems *very* wrong that what we import depends in any way on a previous expression so I agree we should fix that. In theory the ImportDefinition call in the ASTImporter shouldn't do any real work as we have the MinimalImport mode on in LLDB so it should only load some bare bone record with external storage IIUC. So I think the original version of the patch seems like a better approach to me from a quick glance.

In D69933#1830602 <https://reviews.llvm.org/D69933#1830602>, @jarin wrote:

> This is achieved in a hacky way - if we have a complete record R, we pretend it is incomplete by temporarily clearing R's isCompleteDefinition bit. Interestingly, this hack is already used in ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo.


I don't think we do the same hack in the `ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo`. There we forcibly set the complete definition bit of the target to the value of the source (and never touch the source AST). But this code also seems really shady as I can't see why we would ever have to do that unless the import goes wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69933