[Lldb-commits] [PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

Gabor Marton via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Wed Apr 15 08:44:34 PDT 2020

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).



More information about the lldb-commits mailing list