<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Jan 28, 2015, at 3:32 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" class="">richard@metafoo.co.uk</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><div class="gmail_extra"><div class="gmail_quote">On Wed, Jan 28, 2015 at 2:11 PM, Ben Langmuir<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:blangmuir@apple.com" target="_blank" class="">blangmuir@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On Jan 28, 2015, at 1:21 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank" class="">richard@metafoo.co.uk</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class="">Hi Ben,<br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Tue, Jan 27, 2015 at 3:06 PM, Ben Langmuir<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:blangmuir@apple.com" target="_blank" class="">blangmuir@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;">Hi Richard,<br class=""><br class="">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 ;-)</blockquote><div class=""> </div><div class="">LGTM as a diagnostics improvement (but see below).</div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"> <span class="Apple-converted-space"> </span>Don't mistakenly capture byref in block nested in lambda<br class=""><br class=""> <span class="Apple-converted-space"> </span>Even if an enclosing lambda captures a variable by reference, we capture<br class=""> <span class="Apple-converted-space"> </span>it by *copy* in a block unless the variable itself was declared with a<br class=""> <span class="Apple-converted-space"> </span>reference type. Codegen was already correct, but we were not diagnosing<br class=""> <span class="Apple-converted-space"> </span>attempts to modify the variable in Sema.<br class=""></blockquote><div class=""><br class=""></div><div class="">Hmm, I think I'm missing something here</div></div></div></div></div></blockquote></span></div></div></blockquote><div class="">D'oh, what I was missing was the change to captureInBlock!</div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class=""><span class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">-- 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.</div></div></div></div></div></blockquote><div class=""><br class=""></div></span><div class="">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:</div><div class=""><br class=""></div><div class="">int x = 0;</div><div class="">[&]() {</div><div class=""> <span class="Apple-converted-space"> </span>^{</div><div class=""> <span class="Apple-converted-space"> </span>x = 1; // this diagnoses now, but didn’t before</div><div class=""> }();</div><div class="">}();</div></div></div></blockquote><div class=""><br class=""></div><div class="">This part doesn't seem like an obviously-correct change to me. Consider the desugaring of the lambda:</div><div class=""><br class=""></div><div class="">struct Lambda {</div><div class=""> int &x;</div><div class=""> Lambda(int &x) : x(x) {}</div><div class=""> auto operator()() const {</div><div class=""> <span class="Apple-converted-space"> </span>^{</div><div class=""> x = 1;</div><div class=""> <span class="Apple-converted-space"> </span>}();</div><div class=""> }</div><div class="">};</div><div class="">Lambda(x)();</div><div class=""><br class=""></div><div class="">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:</div><div class=""><br class=""></div><div class="">[&x = x]() {</div><div class=""> ^{</div><div class=""> x = 1; // obviously ok, x obviously has reference type</div><div class=""> }();</div><div class="">}();</div><div class=""><br class=""></div><div class="">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.</div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;" class=""><div class=""><span class=""><blockquote type="cite" class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"> <span class="Apple-converted-space"> </span>This also fixes a crash on invalid when trying to modify a variable with<br class=""> <span class="Apple-converted-space"> </span>non-trivial copy constructor and assignment operator in such a<br class=""> <span class="Apple-converted-space"> </span>situation.<br class=""></blockquote><div class=""><br class=""></div><div class="">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.</div></div></div></div></blockquote></span></div><br class=""><div class="">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:</div><div class=""><br class=""></div><div class="">$ cat red.cpp</div><div class="">struct A {<br class=""> A() = default;<br class=""> A(const A &); // non-trivial copy<br class=""> A &operator=(const A &); // non-trivial assign<br class="">};<br class=""><br class="">void test() {<br class=""> A a;<br class=""> [&]() { ^{ (void)a; }(); }();<br class="">}</div><div class=""><br class=""></div><div class="">$ clang++ -std=c++11 -fblocks red.cpp </div></div></blockquote></div><br class=""></div><div class="gmail_extra">Given the above, maybe this is an IRGen bug?</div></div></div></blockquote><br class=""></div><div class="">Sorry for the delayed response. Yes, your argument above is compelling and I’ll put together a new patch.</div><div class=""><br class=""></div><div class="">Ben</div><br class=""></body></html>