<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">On Thu, Mar 26, 2020 at 11:49 PM Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Thu, 26 Mar 2020 at 17:07, David Blaikie via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Thu, Mar 26, 2020 at 3:12 PM Arthur O'Dwyer <<a href="mailto:arthur.j.odwyer@gmail.com" target="_blank">arthur.j.odwyer@gmail.com</a>> wrote:<br></div><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr">I'm not sure, but I do see that the call stack contains a call to<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"></span></p><pre style="color:rgb(0,0,0);font-family:"Courier New",courier,monotype,monospace"><span>bool llvm::function_ref<bool (clang::Expr*&, bool)>::callback_fn<llvm::function_ref<bool (clang::Expr*&, bool)> const>(long, clang::Expr*&, bool)<br></span></pre>Notice that this is function_ref::callback_fn<T> instantiated with T=function_ref itself; i.e., we do have a function_ref to a function_ref. This is the thing I was saying can happen (in my "lamb/sheep" example) but which I thought should never happen in this codepath.<br>Here's function_ref's copy constructor:<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"><br></span></p><p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"> template <typename Callable></span></p>
<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"> function_ref(Callable &&callable,</span></p>
<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"> std::enable_if_t<!std::is_same<std::remove_reference_t<Callable>,</span></p>
<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"> function_ref>::value> * = nullptr)</span></p>
<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"> : callback(callback_fn<typename std::remove_reference<Callable>::type>),</span></p>
<p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"> callable(reinterpret_cast<intptr_t>(&callable)) {}</span></p><p style="margin:0px;font-stretch:normal;font-size:13px;line-height:normal;font-family:Menlo;color:rgb(0,0,0)"><span style="font-variant-ligatures:no-common-ligatures"><br></span></p>I believe that it should be using std::remove_cvref_t, not std::remove_reference_t, so as not to conflict with the compiler's implicitly generated copy constructor.</div><div dir="ltr"><br>Thoughts? [...]</div></div></div></blockquote></div></div></div></div></blockquote><div><br></div><div>OK, so: we're calling the wrong constructor for the inner capture due to the implicit 'const' that's added because the outer lambda is not mutable (and the fix suggested by Arthur is the right one: we should be using remove_cvref_t here not remove_reference_t -- or rather std::remove_cv_t<std::remove_reference_t<Callable>>, since we don't require C++20 yet). And this means that copying a function_ref from a *const* function_ref gives you a new function_ref that refers to the old one, not a new function_ref that refers to the same function the old one did.</div></div></div></blockquote><div><br></div><div>Argh! That is insanely sneaky, and it is quite probable IMHO that this is a core-language bug in GCC, because GCC behaves differently from EDG and Clang here.</div><div></div><div><a href="https://godbolt.org/z/oCvLpv">https://godbolt.org/z/oCvLpv</a><br></div><div>On GCC, when you have a lambda nested within a lambda, and you capture-by-copy a variable which was captured-by-copy using [=] or [i] (but not C++14 init-capture [i=i]), then your data member for that capture will have type `const I`, not just `I`.</div><div>On Clang and EDG, [=], [i], and [i=i] all behave equivalently, and capture a data member with type `I`.</div><div><br></div><div>I don't see anything about this on <a href="https://gcc.gnu.org/bugzilla/buglist.cgi?quicksearch=lambda%20capture%20const">https://gcc.gnu.org/bugzilla/buglist.cgi?quicksearch=lambda%20capture%20const</a> — Richard, you might be best able to describe the issue correctly ;) but if you want me to file it, I will. (Might be a corollary of <a href="https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86697">bug 86697</a>.)</div><div><br></div><div>Regardless, llvm::function_ref's SFINAE should still be fixed. This is a bug in llvm::function_ref, exposed by a bug in GCC. (Or vice versa.)</div><div><br></div><div>–Arthur</div></div></div></div></div></div></div></div>