- User Since
- Jun 24 2018, 6:54 AM (55 w, 3 d)
Sun, Jul 14
This looks fine, but could you please add a test showing how decl shadowing is handled? I.e. if we have Arg in one TU and both Arg and N::Arg in another TU.
This patch looks good to me.
Regarding the related patch: can we use getLambdaManglingNumber() for the comparison?
Sun, Jul 7
The patch looks good, but it looks to me that it has a relation to https://reviews.llvm.org/D64078 that is kind of questionable to me. Let's delay landing this patch until the fix direction is clear.
it is a nice design question if source locations can participate in structural match or not. The comparison tells us that the same code written in different files is not structurally equivalent and I cannot agree with it. They can be not the same, but their structure is the same. The question is: why do we get to this comparison? Do we find a non-equivalent decl by lookup? I guess there can be another way to resolve this issue, and I'll be happy to help if you share what is the problem behind the patch.
Tue, Jul 2
There is an inline question about tests; other code looks fine.
This looks mostly good but I have a question inline.
Sun, Jun 30
The following happened: During the analysis we compared two Decls which turned out to be inequivalent, so we cached them. Later during the analysis, however, we added a new node to the redecl chain of one of these Decls which we previously compared. Then another structural equivalent check followed for the two Decls. And this time they should have been considered structurally equivalent, but the cache already contained them as nonequivalent. This resulted in a false positive NameConflict error.
Should we reset the non-equivalence relation after a decl is imported for this decl and its redecls?
LGTM, thanks for the fixes!
The patch looks good. Thanks!
Thanks for the explanation!
It will be good if someone else takes a look at this patch.
Thank you for the explanation. I got the idea of this patch anyway, but it will be definitely useful for anyone digging into the code. Should we make it a comment for ImportPathTy?
Sun, Jun 23
Jun 3 2019
Looks mostly good, but I have some comments inline.
May 26 2019
I haven't find the import sequence examples we try to fix these ways in any of the three patches these change consists of. Could you please provide some (or point if I missed them)?
Could you please provide any test for the import itself?
The idea looks fine to me, but I have some questions inline.
I have a few questions inline.
Could you provide an example of an import sequence leading to this behavior? It's hard for me to imagine such a situation.
May 19 2019
This looks fine, but I have a question inline.
May 11 2019
The conversion operator indeed looks non-evident.
Wow, this is a cool idea!
Thank you for digging into this! Feel free to ask me if you encounter any problems with the patch.
I like this change! LGTM, just a few nits.
May 4 2019
This looks good to me, but let's wait for LLDB guys to take a look at the patch. Thanks!
Apr 25 2019
Looks good, thanks!
The patch itself is fine, but, as other reviewers pointed, tests are appreciated. I suggest to add a test into ASTImporterTests.cpp - you will find several ways to write tests of different complexity here. I think this change can be tested even with the simplest testImport() facility.
Apr 22 2019
Mmm, what are the pros and cons?
Apr 20 2019
This looks good to me. However, it seems to me that the correct solution is to synthesize a shining new function and include it into the redeclaration chain. This requires much more work, however.
Apr 16 2019
There are some comments inline, but looks promising!
I like the test even more than the change itself!
Apr 8 2019
Mar 28 2019
I think we should accept this change. I don't see an easy way to merge the LLDB stuff into ASTImporter; also, this patch provides a good extension point for ASTImporter since it is designed to be a parent class. @martong @shafik Gabor, Shafik, what do you think?
Yes, I think this is fine. Thanks!
Post-LGTM with some stylish nits.
Thank you for the explanation, it is much more clear to me now. But, as I see, D59692 is going to discard the changes this patch introduces. @martong Gabor, do you expect the changes of this patch to be merged into yours, or should this patch be abandoned?
Thanks for the fixes! I'd like to clarify one moment, however.
Mar 24 2019
Mar 21 2019
Mar 11 2019
This patch LGTM mostly, but there is a comment inline.
Mar 10 2019
Mar 3 2019
Thanks for the patch! It looks good to me except some stylish nits.
The patch looks almost good bu I have some comments inline.
Feb 26 2019
Thanks for the fixes!
Sorry, missed the patch. It looks mostly good, but do we have any test for this?
Feb 24 2019
The patch looks OK overall but I have some comments inline.
I don't see any problems with the patch. Thanks! I think it will be good to get Shafik's approval as well.
Feb 23 2019
Thanks for the fixes! The patch looks good to me now. I have only a small nit inline.
Feb 21 2019
Feb 20 2019
Feb 16 2019
The change looks reasonable but the tests need some improvements.
Thanks for the changes! The patch looks completely fine to me now.
This patch LGTM with a minor nit.
Feb 11 2019
Feb 10 2019
Feb 7 2019
Please find my comments inline.
Looks good, thanks!
Feb 6 2019
Jan 27 2019
There are never enough tests :) Thank you!
Jan 23 2019
Oh, I remember how much pain have debug mode-only assertions caused.
Jan 21 2019
LGTM, thank you!
I'm sorry if the change I requested doesn't look good, but I think the correctness is our priority.
Jan 18 2019
Please find my answers inline.
Jan 14 2019
This looks perfect, just some stylish issues.
Jan 13 2019
The change looks mostly fine but I have some comments inline.
Jan 12 2019
Dec 23 2018
Yes, this looks good. Thanks!
I agree that it doesn't make sense to have 'errors' in AST merging tools, and the changes for ASTImporter part are welcome. However, I don't feel so positive to modules support changes and I don't think we are allowed to change this. @aaron.ballman, @rsmith what do you think?
@bruno, you're the original author of the related Sema code. What do you think, should this be a warning or an error in Sema?
Dec 16 2018
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?
Dec 9 2018
Thank you for the investigation! I'll try to take a look.
The overall idea is good but I have some remarks inline.
Dec 8 2018
I think the change is fine, just a minor stylish remark.
Dec 6 2018
Dec 5 2018
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.
Dec 4 2018
The change looks fine, just some nits.