abhinavgaba added inline comments.
Comment at: clang/unittests/AST/ASTImporterTest.cpp:5974
+ SmallVectorImpl ) override {}
+};
+
Overloading `CompleteType(TagDecl*)` is causing a warning:
virtual void
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdef7c7f60205: [ASTImporter] Fix handling of not defined
FromRecord in ImportContext(...) (authored by shafik).
Herald added projects: clang, LLDB.
Changed prior to commit:
shafik updated this revision to Diff 259703.
shafik marked 2 inline comments as done.
shafik added a comment.
Capitalization fixes.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78000/new/
https://reviews.llvm.org/D78000
Files:
clang/lib/AST/ASTImporter.cpp
a_sidorin accepted this revision.
a_sidorin added a comment.
Hello Shafik!
The patch looks good, I only have a few stylish nits.
Comment at: clang/unittests/AST/ASTImporterTest.cpp:5964
+ new SourceWithCompletedTagList(CompletedTags);
+ clang::ASTContext =
martong accepted this revision.
martong added a comment.
LGTM! Thanks!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78000/new/
https://reviews.llvm.org/D78000
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
shafik updated this revision to Diff 259110.
shafik marked 2 inline comments as done.
shafik added a comment.
- Simplifying the changes in `ASTImporter::ImportContext`
- Removing `DC->setHasExternalLexicalStorage(true);` from unit test since we
are checking `getExternalSource()`
CHANGES SINCE
teemperor added a comment.
Beside Gabors comment I think I'm happy with this. Thanks!
Comment at: clang/unittests/AST/ASTImporterTest.cpp:5939
+
+/// An ExternalASTSource that keeps track of the tags is completed.
+struct SourceWithCompletedTagList : clang::ExternalASTSource {
martong added a comment.
Thanks for the test Shafik, that is pretty self explanatory!
Comment at: clang/lib/AST/ASTImporter.cpp:8161
if (auto *ToRecord = dyn_cast(ToDC)) {
auto *FromRecord = cast(FromDC);
if (ToRecord->isCompleteDefinition()) {
shafik updated this revision to Diff 258084.
shafik added a comment.
- Added unit test
- Modified condition for defining `FromRecord` to whether it has an
`ExternalSource`
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78000/new/
https://reviews.llvm.org/D78000
Files:
shafik marked an inline comment as done.
shafik added inline comments.
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
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
shafik updated this revision to Diff 257552.
shafik marked 4 inline comments as done.
shafik added a comment.
Addressing comments
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78000/new/
https://reviews.llvm.org/D78000
Files:
clang/lib/AST/ASTImporter.cpp
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
shafik added a comment.
I am open to different approaches to this problem, this is opening attempt at
fixing it but I may be missing other interactions.
AFAICT starting the definition of `ToRecord` the way `CompleteDecl(…)` does
when we know `FromRecord` is not defined is just broken for the
shafik created this revision.
shafik added reviewers: martong, balazske, teemperor, a_sidorin.
Herald added a subscriber: rnkovacs.
Herald added a reviewer: a.sidorin.
shafik added a comment.
I am open to different approaches to this problem, this is opening attempt at
fixing it but I may be
15 matches
Mail list logo
