On Fri, Apr 25, 2025 at 12:20 AM Jonathan Wakely <jwak...@redhat.com> wrote:
> Moving the static_assert that checks is_invocable_r_v into _Task_state
> means it is checked when we instantiate that class template.
>
> Replacing the __create_task_state function with a static member function
> _Task_state::_S_create ensures we instantiate _Task_state and trigger
> the static_assert immediately, not deep inside the implementation of
> std::allocate_shared. This results in shorter diagnostics that don't
> show deeply-nested template instantiations before the static_assert
> failure.
>
> Placing the static_assert at class scope also helps us to fail earlier
> than waiting until when the _Task_state::_M_run virtual function is
> instantiated. That also makes the diagnostics shorter and easier to read
> (although for C++11 and C++14 modes the class-scope static_assert
> doesn't check is_invocable_r, so dangling references aren't detected
> until _M_run is instantiated).
>
> libstdc++-v3/ChangeLog:
>
> * include/std/future (__future_base::_Task_state): Check
> invocable requirement here.
> (__future_base::_Task_state::_S_create): New static member
> function.
> (__future_base::_Task_state::_M_reset): Use _S_create.
> (__create_task_state): Remove.
> (packaged_task): Use _Task_state::_S_create instead of
> __create_task_state.
> * testsuite/30_threads/packaged_task/cons/dangling_ref.cc:
> Adjust dg-error patterns.
> * testsuite/30_threads/packaged_task/cons/lwg4154_neg.cc:
> Likewise.
> ---
>
> Tested x86_64-linux.
>
LGTM.
>
> libstdc++-v3/include/std/future | 66 +++++++++----------
> .../packaged_task/cons/dangling_ref.cc | 3 +-
> .../packaged_task/cons/lwg4154_neg.cc | 10 +--
> 3 files changed, 38 insertions(+), 41 deletions(-)
>
> diff --git a/libstdc++-v3/include/std/future
> b/libstdc++-v3/include/std/future
> index b7ab233b85f..080690064a9 100644
> --- a/libstdc++-v3/include/std/future
> +++ b/libstdc++-v3/include/std/future
> @@ -1486,12 +1486,28 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> struct __future_base::_Task_state<_Fn, _Alloc, _Res(_Args...)> final
> : __future_base::_Task_state_base<_Res(_Args...)>
> {
> +#ifdef __cpp_lib_is_invocable // C++ >= 17
> + static_assert(is_invocable_r_v<_Res, _Fn&, _Args...>);
> +#else
> + static_assert(__is_invocable<_Fn&, _Args...>::value,
> + "_Fn& is invocable with _Args...");
> +#endif
> +
> template<typename _Fn2>
> _Task_state(_Fn2&& __fn, const _Alloc& __a)
> : _Task_state_base<_Res(_Args...)>(__a),
> _M_impl(std::forward<_Fn2>(__fn), __a)
> { }
>
> + template<typename _Fn2>
> + static shared_ptr<_Task_state_base<_Res(_Args...)>>
> + _S_create(_Fn2&& __fn, const _Alloc& __a)
> + {
> + return std::allocate_shared<_Task_state>(__a,
> +
> std::forward<_Fn2>(__fn),
> + __a);
> + }
> +
> private:
> virtual void
> _M_run(_Args&&... __args)
> @@ -1515,7 +1531,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> }
>
> virtual shared_ptr<_Task_state_base<_Res(_Args...)>>
> - _M_reset();
> + _M_reset()
> + { return _S_create(std::move(_M_impl._M_fn), _M_impl); }
>
> struct _Impl : _Alloc
> {
> @@ -1525,38 +1542,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> _Fn _M_fn;
> } _M_impl;
> };
> -
> - template<typename _Signature, typename _Fn,
> - typename _Alloc = std::allocator<int>>
> - shared_ptr<__future_base::_Task_state_base<_Signature>>
> - __create_task_state(_Fn&& __fn, const _Alloc& __a = _Alloc())
> - {
> - typedef typename decay<_Fn>::type _Fn2;
> - typedef __future_base::_Task_state<_Fn2, _Alloc, _Signature> _State;
> - return std::allocate_shared<_State>(__a, std::forward<_Fn>(__fn),
> __a);
> - }
> -
> - template<typename _Fn, typename _Alloc, typename _Res, typename...
> _Args>
> - shared_ptr<__future_base::_Task_state_base<_Res(_Args...)>>
> - __future_base::_Task_state<_Fn, _Alloc, _Res(_Args...)>::_M_reset()
> - {
> - return __create_task_state<_Res(_Args...)>(std::move(_M_impl._M_fn),
> -
> static_cast<_Alloc&>(_M_impl));
> - }
> /// @endcond
>
> /// packaged_task
> template<typename _Res, typename... _ArgTypes>
> class packaged_task<_Res(_ArgTypes...)>
> {
> - typedef __future_base::_Task_state_base<_Res(_ArgTypes...)>
> _State_type;
> + using _State_type =
> __future_base::_Task_state_base<_Res(_ArgTypes...)>;
> shared_ptr<_State_type> _M_state;
>
> // _GLIBCXX_RESOLVE_LIB_DEFECTS
> // 3039. Unnecessary decay in thread and packaged_task
> template<typename _Fn, typename _Fn2 = __remove_cvref_t<_Fn>>
> - using __not_same
> - = typename enable_if<!is_same<packaged_task, _Fn2>::value>::type;
> + using __not_same = __enable_if_t<!is_same<packaged_task,
> _Fn2>::value>;
> +
> + // _GLIBCXX_RESOLVE_LIB_DEFECTS
> + // 4154. The Mandates for std::packaged_task's constructor
> + // from a callable entity should consider decaying.
> + template<typename _Fn, typename _Alloc = std::allocator<int>>
> + using _Task_state
> + = __future_base::_Task_state<__decay_t<_Fn>, _Alloc,
> + _Res(_ArgTypes...)>;
>
> public:
> // Construction and destruction
> @@ -1565,16 +1571,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> template<typename _Fn, typename = __not_same<_Fn>>
> explicit
> packaged_task(_Fn&& __fn)
> - : _M_state(
> -
> __create_task_state<_Res(_ArgTypes...)>(std::forward<_Fn>(__fn)))
> - {
> -#ifdef __cpp_lib_is_invocable // C++ >= 17
> - // _GLIBCXX_RESOLVE_LIB_DEFECTS
> - // 4154. The Mandates for std::packaged_task's constructor
> - // from a callable entity should consider decaying
> - static_assert(is_invocable_r_v<_Res, decay_t<_Fn>&,
> _ArgTypes...>);
> -#endif
> - }
> + : _M_state(_Task_state<_Fn>::_S_create(std::forward<_Fn>(__fn),
> {}))
> + { }
>
> #if __cplusplus < 201703L
> // _GLIBCXX_RESOLVE_LIB_DEFECTS
> @@ -1583,8 +1581,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> // 2921. packaged_task and type-erased allocators
> template<typename _Fn, typename _Alloc, typename = __not_same<_Fn>>
> packaged_task(allocator_arg_t, const _Alloc& __a, _Fn&& __fn)
> - : _M_state(__create_task_state<_Res(_ArgTypes...)>(
> - std::forward<_Fn>(__fn), __a))
> + : _M_state(_Task_state<_Fn,
> _Alloc>::_S_create(std::forward<_Fn>(__fn),
> + __a))
> { }
>
> // _GLIBCXX_RESOLVE_LIB_DEFECTS
> diff --git
> a/libstdc++-v3/testsuite/30_threads/packaged_task/cons/dangling_ref.cc
> b/libstdc++-v3/testsuite/30_threads/packaged_task/cons/dangling_ref.cc
> index 51c6ade91c3..8cc3f78da9f 100644
> --- a/libstdc++-v3/testsuite/30_threads/packaged_task/cons/dangling_ref.cc
> +++ b/libstdc++-v3/testsuite/30_threads/packaged_task/cons/dangling_ref.cc
> @@ -8,6 +8,5 @@ int f();
> std::packaged_task<const int&()> task(f);
> // { dg-error "dangling reference" "" { target { c++14_down } } 0 }
> // { dg-error "reference to temporary" "" { target { c++14_down } } 0 }
> -// { dg-error "no matching function" "" { target c++17 } 0 }
> -// { dg-error "enable_if" "" { target c++17 } 0 }
> // { dg-error "static assertion failed" "" { target c++17 } 0 }
> +// { dg-error "note: .*std::is_invocable_r" "" { target c++17 } 0 }
> diff --git
> a/libstdc++-v3/testsuite/30_threads/packaged_task/cons/lwg4154_neg.cc
> b/libstdc++-v3/testsuite/30_threads/packaged_task/cons/lwg4154_neg.cc
> index 6ba1bb1a53e..b3413c2d11a 100644
> --- a/libstdc++-v3/testsuite/30_threads/packaged_task/cons/lwg4154_neg.cc
> +++ b/libstdc++-v3/testsuite/30_threads/packaged_task/cons/lwg4154_neg.cc
> @@ -12,16 +12,16 @@ struct F {
>
> // Mandates: is_invocable_r_v<R, decay_t<F>&, ArgTypes...> is true.
> const F f;
> -std::packaged_task<void()> p(f); // { dg-error "here" "" { target c++17 }
> }
> -// { dg-error "static assertion failed" "" { target c++17 } 0 }
> -// { dg-error "invoke_r" "" { target *-*-* } 0 }
> -// { dg-prune-output "enable_if<false" }
> +std::packaged_task<void()> p(f); // { dg-error "here" }
> +// { dg-error "static assertion failed" "" { target *-*-* } 0 }
> +// { dg-error "note: .*std::is_invocable_r_v<void, " "" { target c++17 }
> 0 }
>
> // Only callable as rvalue
> struct Frv {
> int* operator()() && { return 0; }
> };
> -std::packaged_task<int*()> p2(Frv{}); // { dg-error "here" "" { target
> c++17 } }
> +std::packaged_task<int*()> p2(Frv{}); // { dg-error "here" }
> +// { dg-error "note: .*std::is_invocable_r_v<int., " "" { target c++17 }
> 0 }
>
> // Only callable as non-const lvalue
> struct Fnc {
> --
> 2.49.0
>
>
