On Wed, Jan 28, 2015 at 2:11 PM, Ben Langmuir <[email protected]> wrote:
> > On Jan 28, 2015, at 1:21 PM, Richard Smith <[email protected]> wrote: > > Hi Ben, > > On Tue, Jan 27, 2015 at 3:06 PM, Ben Langmuir <[email protected]> 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?
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
