- User Since
- Jun 24 2018, 6:54 AM (25 w, 3 d)
Sun, Dec 16
Hi Gabor, thank you a lot!
I have to say that even having your patch, I still don't understand why the patch fixes the problem (and what the problem is). As I see, the patch handles the cases where some fields or friends have DeclContext different from ToRD. Could you please provide examples so I can add them to the test suite or, at least, just investigate?
Sun, Dec 9
Thank you for the investigation! I'll try to take a look.
The overall idea is good but I have some remarks inline.
Sat, Dec 8
I think the change is fine, just a minor stylish remark.
Thu, Dec 6
Wed, Dec 5
There is a code in getExternalAST:
std::unique_ptr<ASTUnit> LoadedUnit(ASTUnit::LoadFromASTFile( ASTFileName, CI.getPCHContainerOperations()->getRawReader(), ASTUnit::LoadEverything, Diags, CI.getFileSystemOpts())); Unit = LoadedUnit.get(); FileASTUnitMap[ASTFileName] = std::move(LoadedUnit);
And ASTUnit::LoadFromASTFile()can return nullptr. Actually, there is a problem in loadExternalAST() - it ignores this fact.
Tue, Dec 4
The change looks fine, just some nits.
Mon, Dec 3
Sat, Dec 1
Please find my comments inline.
In addition to Umann remarks, there is a small comment inline.
Please find my comments inline.
Sorry, but I don't understand the meaning of some options. Could you please explain what are NumNoUnit and NumNotInOtherTU and what is the difference between them?
More assertions are always good.
Yes, the %T variable is obsolete, but it looks like the Guide recommendations are not entirely fulfilled.
LGTM with a nit: the call to GetAlreadyImportedOrNull is not removed but moved into ImportDeclParts.
Thank you for digging into this! Unfortunately, I don't have any MacOS device and I cannot check my code for the compatibility.
I think the code looks good. Thank you!
Wed, Nov 28
Here are some new comments.
I've already done check-all (this includes LLDB) with this patch. As you can see in this thread, I didn't manage to reproduce some issues on Linux. Luckily, one test still failed on Linux so I could find the problem. However , it will be good to ensure that this is the same problem that caused failures on MacOS and the commit will be safe. That's why I asked for an additional check.
Mon, Nov 26
Sun, Nov 25
More constants are always welcome.
This change looks fine to me but please wait for LLDB reviewer's decision.
The changes look mostly fine to me. I think they can be accepted after some minor stylish fixes.
LGTM. Thank you for addressing my questions!
Nov 18 2018
Nov 14 2018
The change looks fine. Thanks!
Oct 30 2018
Thank you for the patch. The reason for this change looks clear. However, I don't think omitting this comparison completely is what we want here. Instead, we can do a dance similar to ASTContext::mergeFunctionTypes() where all attributes but NoReturn are compared. What do you think?
Another version has been committed (D51633).
Thank you for the explanation!
Oops. Thank you Davide!
Oct 27 2018
Oct 26 2018
I wonder if it is possible to get into situation where non-equivalent decls are marked equivalent with this patch? If yes, we can create a mapping between decls being imported and original decls as an alternative solution. However, I cannot find any counterexample.
I have tried to rebase it last week but there were some test failures. I hope to fix them this weekend and finally commit the patch.
Thank you for the patch. I looks reasonable but I have some questions.
Oct 18 2018
I cannot find any problems with the latest changes. I suggest you to commit the patch to make further reviews easier.
Oct 14 2018
Sep 30 2018
It's going to take some time to review the whole patch.
Sep 16 2018
Sep 15 2018
Sep 9 2018
The change looks mostly fine but the difference with ASTReader approach disturbs me a bit.
Sep 2 2018
Aug 23 2018
Aug 22 2018
Thank you for working on this!
Wow, I totally overlooked this. Thank you!
Aug 21 2018
Aug 20 2018
These methods will be really useful.
Aug 19 2018
Aug 15 2018
As a side note: It seems this test case actually reveals that we don't import the body of Foo's destructor?
This is strange. If you manage to find the reason, please notify us!
Hello Gabor and Balazs,
Tests are always welcome. Thanks!
Aug 13 2018
All declarations are reordered now, not only fields. Also some review comments were addressed.
Aug 11 2018
Aug 8 2018
Yes, this seems to be correct. Thanks!
LGTM! Just a stylish nit.
Aug 3 2018
The approach is OK but I have some minor comments inline.
Aug 1 2018
LGTM. Thank you!
Jul 16 2018
Thank you Gabor!
Jul 14 2018
The change is OK but I have some questions regarding tests.
Could you provide some tests for the issue?
Adding new nodes is always welcome.
LGTM. Just some stylish nits.
To resolve this issue, I used Sema::DeclareImplicit... methods. But I like this approach much more because it doesn't allows to forget different kinds of implicit methods and doesn't require passing Sema into ASTImporter.