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

[Gabor Marton via Phabricator via lldb-commits]

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