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

[Raphael Isemann via Phabricator via cfe-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

>From what I understand the whole idea here is to just ask the external AST source to complete the record before we import them? If yes, then this seems like the right idea to me.

Also this seems to be testable via a Clang unit test, so I think this patch should have one.

But otherwise this LGTM. Thanks for figuring this out!



================
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() &&
----------------
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.


================
Comment at: clang/lib/AST/ASTImporter.cpp:8180
+      if (Error Err = ASTNodeImporter(*this).ImportDefinition(
+              FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
+        return std::move(Err);
----------------
I assume we should check here that FromRecord is now a complete definition before trying to import it's definition?


================
Comment at: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp:25
+    return true; //%self.expect_expr("DD->dump()", result_type="int",
+                 // result_value="42")
+  }
----------------
You need to make this a "//%" as otherwise this test fails (which it does right now).


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

https://reviews.llvm.org/D78000