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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
