[PATCH] Don't mistakenly capture byref in block nested in lambda

[Ben Langmuir]

> On Jan 28, 2015, at 3:32 PM, Richard Smith <richard at metafoo.co.uk <mailto:richard at metafoo.co.uk>> wrote:
> 
> On Wed, Jan 28, 2015 at 2:11 PM, Ben Langmuir <blangmuir at apple.com <mailto:blangmuir at apple.com>> wrote:
> 
>> On Jan 28, 2015, at 1:21 PM, Richard Smith <richard at metafoo.co.uk <mailto:richard at metafoo.co.uk>> wrote:
>> 
>> Hi Ben,
>> 
>> On Tue, Jan 27, 2015 at 3:06 PM, Ben Langmuir <blangmuir at apple.com <mailto:blangmuir at apple.com>> wrote:
>> Hi Richard,
>> 
>> Could you take a look at this patch?  I haven’t messed with capturing in a long time and I don’t want to break it ;-)
>>  
>> LGTM as a diagnostics improvement (but see below).
>>  
>>     Don't mistakenly capture byref in block nested in lambda
>> 
>>     Even if an enclosing lambda captures a variable by reference, we capture
>>     it by *copy* in a block unless the variable itself was declared with a
>>     reference type. Codegen was already correct, but we were not diagnosing
>>     attempts to modify the variable in Sema.
>> 
>> Hmm, I think I'm missing something here
> 
> D'oh, what I was missing was the change to captureInBlock!
>> -- it looks like we would diagnose it, but we'd use a suboptimal diagnostic: CheckForModifiableLvalue would diagnose as err_typecheck_assign_const in this case.
> 
> It wouldn’t be const at all, because we created the (bogus) byref capture.  It occurs to me that my tests don’t make this obvious, so I’ll add:
> 
> int x = 0;
> [&]() {
>   ^{
>     x = 1; // this diagnoses now, but didn’t before
>    }();
> }();
> 
> This part doesn't seem like an obviously-correct change to me. Consider the desugaring of the lambda:
> 
> struct Lambda {
>   int &x;
>   Lambda(int &x) : x(x) {}
>   auto operator()() const {
>     ^{
>     x = 1;
>     }();
>   }
> };
> Lambda(x)();
> 
> In this world, the block is referring to an entity with reference type, so it should capture by reference instead of by value. As far as I can see, there's no other way of observing this difference, so we don't have a lot of guidance here, but treating 'x' like a variable of reference type seems more consistent here. Especially when you consider:
> 
> [&x = x]() {
>   ^{
>   x = 1; // obviously ok, x obviously has reference type
>   }();
> }();
> 
> There are some strong proponents of the idea that [&x] should be exactly equivalent to [&x = x], and that [x] should be exactly equivalent to [x = x]; they *very nearly* are equivalent (they only differ in cv-qualifiers today); this change gets us further from that position.
>>     This also fixes a crash on invalid when trying to modify a variable with
>>     non-trivial copy constructor and assignment operator in such a
>>     situation.
>> 
>> I can't reproduce this crash with current trunk, assuming this is supposed to be covered by the uses of 'A' in test7 in SemaCXX/blocks.cpp.
> 
> Ah my mistake, the crash was reading from the value, not writing to it.  I’ll remove that ‘A’ case in the Sema test and add an IRGen test to exercise it.  If you want to reproduce it:
> 
> $ cat red.cpp
> struct A {
>   A() = default;
>   A(const A &); // non-trivial copy
>   A &operator=(const A &); // non-trivial assign
> };
> 
> void test() {
>   A a;
>   [&]() { ^{ (void)a; }(); }();
> }
> 
> $ clang++ -std=c++11 -fblocks red.cpp 
> 
> Given the above, maybe this is an IRGen bug?

Sorry for the delayed response. Yes, your argument above is compelling and I’ll put together a new patch.

Ben

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150203/6a4a8950/attachment.html>