On Thu, Mar 26, 2020 at 11:49 PM Richard Smith <rich...@metafoo.co.uk>
wrote:
> On Thu, 26 Mar 2020 at 17:07, David Blaikie via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> On Thu, Mar 26, 2020 at 3:12 PM Arthur O'Dwyer <arthur.j.odw...@gmail.com>
>> wrote:
>>
>>> I'm not sure, but I do see that the call stack contains a call to
>>>
>>> bool llvm::function_ref<bool (clang::Expr*&,
>>> bool)>::callback_fn<llvm::function_ref<bool (clang::Expr*&, bool)>
>>> const>(long, clang::Expr*&, bool)
>>>
>>> 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.
>>> Here's function_ref's copy constructor:
>>>
>>>
>>> template <typename Callable>
>>>
>>> function_ref(Callable &&callable,
>>>
>>>
>>> std::enable_if_t<!std::is_same<std::remove_reference_t<Callable>,
>>>
>>> function_ref>::value> * =
>>> nullptr)
>>>
>>> : callback(callback_fn<typename
>>> std::remove_reference<Callable>::type>),
>>>
>>> callable(reinterpret_cast<intptr_t>(&callable)) {}
>>>
>>>
>>> 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.
>>>
>>> Thoughts? [...]
>>>
>>
> 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.
>
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.
https://godbolt.org/z/oCvLpv
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`.
On Clang and EDG, [=], [i], and [i=i] all behave equivalently, and capture
a data member with type `I`.
I don't see anything about this on
https://gcc.gnu.org/bugzilla/buglist.cgi?quicksearch=lambda%20capture%20const
— 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 bug 86697
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86697>.)
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.)
–Arthur
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
