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

[Richard Smith]

On Wed, Jan 28, 2015 at 2:11 PM, Ben Langmuir <blangmuir at apple.com> wrote:

>
> On Jan 28, 2015, at 1:21 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
> Hi Ben,
>
> On Tue, Jan 27, 2015 at 3:06 PM, Ben Langmuir <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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150128/a42d987d/attachment.html>