martong added a comment.

In D64078#1572675 <https://reviews.llvm.org/D64078#1572675>, @a_sidorin wrote:

> Hi Gabor,
>  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.


Hey Alexei, thanks for looking into this. I agree, that those lambdas are 
actually "structurally" equivalent even if they are defined in different 
locations...
But still we have to differentiate them somehow.
Consider this code:

  void f() {
    auto L0 = [](){};
    auto L1 = [](){};
  }

First we import `L0` then `L1`. Currently we end up having only one 
CXXRecordDecl for the two different lambdas. And that is a problem if the body 
of their op() is different.
This happens because when we import `L1` then lookup finds the existing `L0` 
and since they are structurally equivalent we just map the imported L0 to be 
the counterpart of L1 <https://reviews.llvm.org/L1>.

I tried to use `getLambdaManglingNumber()` as you suggested in 
https://reviews.llvm.org/D64075 and that does solve this particular case, but 
cannot solve to other issue we have in the test case 
`LambdasInFunctionParamsAreDifferentiated`:

  template <typename F0, typename F1>
  void f(F0 L0 = [](){}, F1 L1 = [](){}) {}

Here after importing L0, L1 <https://reviews.llvm.org/L1> is mapped to L0 again.

I tried an alternative solution, which is to disable lookup completely if the 
decl in the "from" context is a lambda. That passed all tests.
However, that could have other problems: what if the lambda is defined in a 
header file and included in several TUs? I think we'd have as many duplicates 
as many includes we have. I think we could live with that, because the lambda 
classes are TU local anyway, we cannot just access them from another TU.
I have created another patch for that solution: https://reviews.llvm.org/D66348


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64078



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to