Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

<future>: Make packaged_task accept move-only functors #4946

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+160 −32
Open

<future>: Make packaged_task accept move-only functors #4946

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5195dd0
Make `packaged_task` accept move-only functors
frederick-vs-ja Sep 10, 2024
a57cf70
Constrain constructor overloads to keep behavior as possible
frederick-vs-ja Sep 10, 2024
edc3b57
It's incorrect to use `decay_t` for [futures.task.members]/3
frederick-vs-ja Sep 10, 2024
5e3ea4e
Merge branch 'microsoft:main' into packaged-move-only-task
frederick-vs-ja Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions stl/inc/functional
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -767,7 +767,10 @@ public:
private:
_Mybase* _Copy(void* _Where) const override {
auto& _Myax = _Mypair._Get_first();
if constexpr (_Is_large<_Func_impl>) {
if constexpr (!is_copy_constructible_v<_Callable>) { // used exclusively for packaged_task
(void) _Myax;
_CSTD abort(); // shouldn't be called, see GH-3888
} else if constexpr (_Is_large<_Func_impl>) {
_Myalty _Rebound(_Myax);
_Alloc_construct_ptr<_Myalty> _Constructor{_Rebound};
_Constructor._Allocate();
Expand Down Expand Up @@ -854,7 +857,9 @@ public:

private:
_Mybase* _Copy(void* _Where) const override {
if constexpr (_Is_large<_Func_impl_no_alloc>) {
if constexpr (!is_copy_constructible_v<_Callable>) { // used exclusively for packaged_task
_CSTD abort(); // shouldn't be called, see GH-3888
} else if constexpr (_Is_large<_Func_impl_no_alloc>) {
return _STD _Global_new<_Func_impl_no_alloc>(_Callee);
} else {
return ::new (_Where) _Func_impl_no_alloc(_Callee);
Expand Down Expand Up @@ -1070,6 +1075,10 @@ _NON_MEMBER_CALL(_GET_FUNCTION_IMPL_NOEXCEPT, X1, X2, X3)
#undef _GET_FUNCTION_IMPL_NOEXCEPT
#endif // defined(__cpp_noexcept_function_type)

struct _Secret_copyability_ignoring_tag { // used exclusively for packaged_task
explicit _Secret_copyability_ignoring_tag() = default;
};

_EXPORT_STD template <class _Fty>
class function : public _Get_function_impl<_Fty>::type { // wrapper for callable objects
private:
Expand All @@ -1086,6 +1095,14 @@ public:

template <class _Fx, typename _Mybase::template _Enable_if_callable_t<_Fx, function> = 0>
function(_Fx&& _Func) {
static_assert(is_copy_constructible_v<decay_t<_Fx>>,
"The target function object type must be copy constructible (N4988 [func.wrap.func.con]/10.1).");
this->_Reset(_STD forward<_Fx>(_Func));
}

template <class _SecretTag, class _Fx, typename _Mybase::template _Enable_if_callable_t<_Fx, function> = 0,
enable_if_t<is_same_v<_SecretTag, _Secret_copyability_ignoring_tag>, int> = 0>
explicit function(_SecretTag, _Fx&& _Func) { // used exclusively for packaged_task
Comment on lines +1103 to +1105
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is _SecretTag a template parameter? Could it be just a parameter of type _Secret_copyability_ignoring_tag ?

Copy link
Contributor Author

@frederick-vs-ja frederick-vs-ja Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is necessary for defending against some convertible-to-almost-all types. But perhaps we don't need it as of C++17 since the allocator_arg_t ctors are removed... although users can restore them.

Copy link
Member

@CaseyCarter CaseyCarter Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _SecretTag constraint should probably be first so we avoid evaluating the other constraint when it's not satisfied.

this->_Reset(_STD forward<_Fx>(_Func));
}

Expand All @@ -1103,6 +1120,16 @@ public:

template <class _Fx, class _Alloc, typename _Mybase::template _Enable_if_callable_t<_Fx, function> = 0>
function(allocator_arg_t, const _Alloc& _Ax, _Fx&& _Func) {
static_assert(is_copy_constructible_v<decay_t<_Fx>>,
"The target function object type must be copy constructible (N4140 [func.wrap.func.con]/7).");
this->_Reset_alloc(_STD forward<_Fx>(_Func), _Ax);
}

template <class _SecretTag, class _Fx, class _Alloc,
typename _Mybase::template _Enable_if_callable_t<_Fx, function> = 0,
enable_if_t<is_same_v<_SecretTag, _Secret_copyability_ignoring_tag>, int> = 0>
explicit function(_SecretTag, allocator_arg_t, const _Alloc& _Ax, _Fx&& _Func) {
// used exclusively for packaged_task
this->_Reset_alloc(_STD forward<_Fx>(_Func), _Ax);
}
#endif // _HAS_FUNCTION_ALLOCATOR_SUPPORT
Expand Down
112 changes: 83 additions & 29 deletions stl/inc/future
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,16 +466,29 @@ template <class _Ret, class... _ArgTypes>
class _Packaged_state<_Ret(_ArgTypes...)>
: public _Associated_state<_Ret> { // class for managing associated asynchronous state for packaged_task
public:
using _Mybase = _Associated_state<_Ret>;
using _Mydel = typename _Mybase::_Mydel;
using _Mybase = _Associated_state<_Ret>;
using _Mydel = typename _Mybase::_Mydel;
using _Function_type = function<_Ret(_ArgTypes...)>;

template <class _Fty2>
_Packaged_state(_Fty2&& _Fnarg) : _Fn(_STD forward<_Fty2>(_Fnarg)) {}
explicit _Packaged_state(const _Function_type& _Fnarg) : _Fn(_Fnarg) {}

explicit _Packaged_state(_Function_type&& _Fnarg) noexcept : _Fn(_STD move(_Fnarg)) {}

template <class _Fty2, enable_if_t<!is_same_v<_Remove_cvref_t<_Fty2>, _Function_type>, int> = 0>
explicit _Packaged_state(_Fty2&& _Fnarg) : _Fn(_Secret_copyability_ignoring_tag{}, _STD forward<_Fty2>(_Fnarg)) {}

#if _HAS_FUNCTION_ALLOCATOR_SUPPORT
template <class _Fty2, class _Alloc>
template <class _Alloc>
_Packaged_state(const _Function_type& _Fnarg, const _Alloc& _Al, _Mydel* _Dp)
: _Mybase(_Dp), _Fn(allocator_arg, _Al, _Fnarg) {}

template <class _Alloc>
_Packaged_state(_Function_type&& _Fnarg, const _Alloc& _Al, _Mydel* _Dp)
: _Mybase(_Dp), _Fn(allocator_arg, _Al, _STD move(_Fnarg)) {}

template <class _Fty2, class _Alloc, enable_if_t<!is_same_v<_Remove_cvref_t<_Fty2>, _Function_type>, int> = 0>
_Packaged_state(_Fty2&& _Fnarg, const _Alloc& _Al, _Mydel* _Dp)
: _Mybase(_Dp), _Fn(allocator_arg, _Al, _STD forward<_Fty2>(_Fnarg)) {}
: _Mybase(_Dp), _Fn(_Secret_copyability_ignoring_tag{}, allocator_arg, _Al, _STD forward<_Fty2>(_Fnarg)) {}
#endif // _HAS_FUNCTION_ALLOCATOR_SUPPORT

void _Call_deferred(_ArgTypes... _Args) { // set deferred call
Expand All @@ -498,28 +511,44 @@ public:
_CATCH_END
}

const auto& _Get_fn() const {
const auto& _Get_fn() const& {
return _Fn;
}
auto&& _Get_fn() && noexcept {
return _STD move(_Fn);
}

private:
function<_Ret(_ArgTypes...)> _Fn;
_Function_type _Fn;
};

template <class _Ret, class... _ArgTypes>
class _Packaged_state<_Ret&(_ArgTypes...)>
: public _Associated_state<_Ret*> { // class for managing associated asynchronous state for packaged_task
public:
using _Mybase = _Associated_state<_Ret*>;
using _Mydel = typename _Mybase::_Mydel;
using _Mybase = _Associated_state<_Ret*>;
using _Mydel = typename _Mybase::_Mydel;
using _Function_type = function<_Ret&(_ArgTypes...)>;

template <class _Fty2>
_Packaged_state(_Fty2&& _Fnarg) : _Fn(_STD forward<_Fty2>(_Fnarg)) {}
explicit _Packaged_state(const _Function_type& _Fnarg) : _Fn(_Fnarg) {}

explicit _Packaged_state(_Function_type&& _Fnarg) noexcept : _Fn(_STD move(_Fnarg)) {}

template <class _Fty2, enable_if_t<!is_same_v<_Remove_cvref_t<_Fty2>, _Function_type>, int> = 0>
explicit _Packaged_state(_Fty2&& _Fnarg) : _Fn(_Secret_copyability_ignoring_tag{}, _STD forward<_Fty2>(_Fnarg)) {}

#if _HAS_FUNCTION_ALLOCATOR_SUPPORT
template <class _Fty2, class _Alloc>
template <class _Alloc>
_Packaged_state(const _Function_type& _Fnarg, const _Alloc& _Al, _Mydel* _Dp)
: _Mybase(_Dp), _Fn(allocator_arg, _Al, _Fnarg) {}

template <class _Alloc>
_Packaged_state(_Function_type&& _Fnarg, const _Alloc& _Al, _Mydel* _Dp)
: _Mybase(_Dp), _Fn(allocator_arg, _Al, _STD move(_Fnarg)) {}

template <class _Fty2, class _Alloc, enable_if_t<!is_same_v<_Remove_cvref_t<_Fty2>, _Function_type>, int> = 0>
_Packaged_state(_Fty2&& _Fnarg, const _Alloc& _Al, _Mydel* _Dp)
: _Mybase(_Dp), _Fn(allocator_arg, _Al, _STD forward<_Fty2>(_Fnarg)) {}
: _Mybase(_Dp), _Fn(_Secret_copyability_ignoring_tag{}, allocator_arg, _Al, _STD forward<_Fty2>(_Fnarg)) {}
#endif // _HAS_FUNCTION_ALLOCATOR_SUPPORT

void _Call_deferred(_ArgTypes... _Args) { // set deferred call
Expand All @@ -542,28 +571,44 @@ public:
_CATCH_END
}

const auto& _Get_fn() const {
const auto& _Get_fn() const& {
return _Fn;
}
auto&& _Get_fn() && noexcept {
return _STD move(_Fn);
}

private:
function<_Ret&(_ArgTypes...)> _Fn;
_Function_type _Fn;
};

template <class... _ArgTypes>
class _Packaged_state<void(_ArgTypes...)>
: public _Associated_state<int> { // class for managing associated asynchronous state for packaged_task
public:
using _Mybase = _Associated_state<int>;
using _Mydel = typename _Mybase::_Mydel;
using _Mybase = _Associated_state<int>;
using _Mydel = typename _Mybase::_Mydel;
using _Function_type = function<void(_ArgTypes...)>;

template <class _Fty2>
_Packaged_state(_Fty2&& _Fnarg) : _Fn(_STD forward<_Fty2>(_Fnarg)) {}
explicit _Packaged_state(const _Function_type& _Fnarg) : _Fn(_Fnarg) {}

explicit _Packaged_state(_Function_type&& _Fnarg) noexcept : _Fn(_STD move(_Fnarg)) {}

template <class _Fty2, enable_if_t<!is_same_v<_Remove_cvref_t<_Fty2>, _Function_type>, int> = 0>
explicit _Packaged_state(_Fty2&& _Fnarg) : _Fn(_Secret_copyability_ignoring_tag{}, _STD forward<_Fty2>(_Fnarg)) {}

#if _HAS_FUNCTION_ALLOCATOR_SUPPORT
template <class _Fty2, class _Alloc>
template <class _Alloc>
_Packaged_state(const _Function_type& _Fnarg, const _Alloc& _Al, _Mydel* _Dp)
: _Mybase(_Dp), _Fn(allocator_arg, _Al, _Fnarg) {}

template <class _Alloc>
_Packaged_state(_Function_type&& _Fnarg, const _Alloc& _Al, _Mydel* _Dp)
: _Mybase(_Dp), _Fn(allocator_arg, _Al, _STD move(_Fnarg)) {}

template <class _Fty2, class _Alloc, enable_if_t<!is_same_v<_Remove_cvref_t<_Fty2>, _Function_type>, int> = 0>
_Packaged_state(_Fty2&& _Fnarg, const _Alloc& _Al, _Mydel* _Dp)
: _Mybase(_Dp), _Fn(allocator_arg, _Al, _STD forward<_Fty2>(_Fnarg)) {}
: _Mybase(_Dp), _Fn(_Secret_copyability_ignoring_tag{}, allocator_arg, _Al, _STD forward<_Fty2>(_Fnarg)) {}
#endif // _HAS_FUNCTION_ALLOCATOR_SUPPORT

void _Call_deferred(_ArgTypes... _Args) { // set deferred call
Expand All @@ -588,12 +633,15 @@ public:
_CATCH_END
}

const auto& _Get_fn() const {
const auto& _Get_fn() const& {
return _Fn;
}
auto&& _Get_fn() && noexcept {
return _STD move(_Fn);
}

private:
function<void(_ArgTypes...)> _Fn;
_Function_type _Fn;
};

template <class _Ty, class _Alloc>
Expand Down Expand Up @@ -1266,7 +1314,10 @@ public:
packaged_task() = default;

template <class _Fty2, enable_if_t<!is_same_v<_Remove_cvref_t<_Fty2>, packaged_task>, int> = 0>
explicit packaged_task(_Fty2&& _Fnarg) : _MyPromise(new _MyStateType(_STD forward<_Fty2>(_Fnarg))) {}
explicit packaged_task(_Fty2&& _Fnarg) : _MyPromise(new _MyStateType(_STD forward<_Fty2>(_Fnarg))) {
static_assert(_Is_invocable_r<_Ret, _Fty2&, _ArgTypes...>::value,
"The function object must be callable with _ArgTypes... and return _Ret (N4988 [futures.task.members]/3).");
Comment on lines +1318 to +1319
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the change is exactly reflecting [futures.task.members]/3, but the Mandates doesn't seem very correct when remove_reference_t<_Fty2> is cv-qualified.
Should we use decay_t<_Fty2>& or _Remove_cvref_t<_Fty2>&? I've mailed to LWG Chair to submit an LWG issue to say decay_t in [futures.task.members]/3.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LWG4154

}

packaged_task(packaged_task&&) noexcept = default;

Expand All @@ -1275,7 +1326,10 @@ public:
#if _HAS_FUNCTION_ALLOCATOR_SUPPORT
template <class _Fty2, class _Alloc, enable_if_t<!is_same_v<_Remove_cvref_t<_Fty2>, packaged_task>, int> = 0>
packaged_task(allocator_arg_t, const _Alloc& _Al, _Fty2&& _Fnarg)
: _MyPromise(_STD _Make_packaged_state<_MyStateType>(_STD forward<_Fty2>(_Fnarg), _Al)) {}
: _MyPromise(_STD _Make_packaged_state<_MyStateType>(_STD forward<_Fty2>(_Fnarg), _Al)) {
static_assert(_Is_invocable_r<_Ret, _Fty2&, _ArgTypes...>::value,
"The function object must be callable with _ArgTypes... and return _Ret (N4140 [futures.task.members]/2).");
}
#endif // _HAS_FUNCTION_ALLOCATOR_SUPPORT

~packaged_task() noexcept {
Expand Down Expand Up @@ -1319,9 +1373,9 @@ public:
}

void reset() { // reset to newly constructed state
_MyStateManagerType& _State = _MyPromise._Get_state_for_set();
_MyStateType* _MyState = static_cast<_MyStateType*>(_State._Ptr());
_MyPromiseType _New_promise(new _MyStateType(_MyState->_Get_fn()));
_MyStateManagerType& _State_mgr = _MyPromise._Get_state_for_set();
_MyStateType& _MyState = *static_cast<_MyStateType*>(_State_mgr._Ptr());
_MyPromiseType _New_promise(new _MyStateType(_STD move(_MyState)._Get_fn()));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's perhaps necessary to move per [futures.task.members]/26. However, the behavior of formerly valid code will change. Not sure whether this is acceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems possible to keep old behavior for existing types while enable correct behavior for newly allowed move-only functors - make the new branch of _Copy do const_cast and then call _Move. But that's somehow weird.

_MyPromise._Get_state()._Abandon();
_MyPromise._Swap(_New_promise);
}
Expand Down
11 changes: 11 additions & 0 deletions tests/std/tests/Dev10_561430_list_and_tree_leaks/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,17 @@ int main() {
assert(f.get() == 1234);
}

// Also test GH-321: "<future>: packaged_task can't be constructed from a move-only lambda"
{
packaged_task<int()> pt(allocator_arg, Mallocator<int>(), [uptr = make_unique<int>(172)] { return *uptr; });

future<int> f = pt.get_future();

pt();

assert(f.get() == 172);
}

{
int n = 4096;

Expand Down
15 changes: 15 additions & 0 deletions tests/std/tests/Dev11_0235721_async_and_packaged_task/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ void test_DevDiv_725337() {
int i = 1729;
auto ref_lambda = [&]() -> int& { return i; };

// GH-321: "<future>: packaged_task can't be constructed from a move-only lambda"
auto move_only_lambda = [uptr = make_unique<int>(42)] { return *uptr; };

{
packaged_task<int()> pt1([] { return 19937; });
future<int> f = pt1.get_future();
Expand All @@ -90,6 +93,18 @@ void test_DevDiv_725337() {
assert(f.get() == 19937);
}

{
packaged_task<int()> pt1(move(move_only_lambda));
future<int> f = pt1.get_future();
packaged_task<int()> pt2(move(pt1));
packaged_task<int()> pt3;
pt3 = move(pt2);
assert(f.wait_for(0s) == future_status::timeout);
pt3();
assert(f.wait_for(0s) == future_status::ready);
assert(f.get() == 42);
}

{
packaged_task<int&()> pt1(ref_lambda);
future<int&> f = pt1.get_future();
Expand Down
1 change: 0 additions & 1 deletion tests/std/tests/GH_000140_adl_proof_construction/test.compile.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,6 @@ void test_function() {

void test_packaged_task() {
packaged_task<void(validator)>{};
packaged_task<void(validator)>{nullptr};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line was wrong per [futures.task.members]/3 but I didn't realize that when it was added.

packaged_task<void(validator)>{simple_identity{}};
packaged_task<void(validator)>{simple_large_identity{}};

Expand Down
18 changes: 18 additions & 0 deletions tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,24 @@ void run_tests() {

assert(f.get().x == 7);
}

// Also test GH-321: "<future>: packaged_task can't be constructed from a move-only lambda"
{
struct MoveOnlyFunctor {
MoveOnlyFunctor() = default;
MoveOnlyFunctor(MoveOnlyFunctor&&) = default;
MoveOnlyFunctor& operator=(MoveOnlyFunctor&&) = default;

T operator()() const {
return T{172};
}
};
std::packaged_task<T()> pt(MoveOnlyFunctor{});
Future f = pt.get_future();
pt();

assert(f.get().x == 172);
}
}

int main() {
Expand Down
4 changes: 4 additions & 0 deletions tests/std/tests/VSO_0000000_instantiate_iterators_misc/test.compile.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,9 +554,13 @@ void future_test() {
swap_test(pv);

packaged_task<void()> pt([]() {});
// GH-321: "<future>: packaged_task can't be constructed from a move-only lambda"
packaged_task<void()> pt2([uptr = unique_ptr<int>{}]() { (void) uptr; });

#if _HAS_FUNCTION_ALLOCATOR_SUPPORT
packaged_task<void()> pta(allocator_arg, allocator<double>{}, []() {});
// GH-321: "<future>: packaged_task can't be constructed from a move-only lambda"
packaged_task<void()> pta2(allocator_arg, allocator<double>{}, [uptr = unique_ptr<int>{}]() { (void) uptr; });
#endif // _HAS_FUNCTION_ALLOCATOR_SUPPORT

swap_test(pt);
Expand Down