Skip to content

Make some special member functions of promise and packaged_task defaulted #3315

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

Merged
Stephan T. Lavavej (StephanTLavavej) merged 3 commits into from
Jan 12, 2023
Merged

Make some special member functions of promise and packaged_task defaulted #3315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
52b4119
Also test exception specifications for `promise`
frederick-vs-ja Dec 31, 2022
bec8946
Also test exception specifications for `packaged_task`
frederick-vs-ja Dec 31, 2022
fe871f9
Defaulted special member functions for `promise` and `packaged_task`
frederick-vs-ja Dec 31, 2022
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
49 changes: 20 additions & 29 deletions stl/inc/future
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ public:
}

// TRANSITION: `_Retrieved` should be assigned before `_Exception` is thrown so that a `future::get`
// that throws a stored exception invalidates the future (N4917 [futures.unique.future]/17)
// that throws a stored exception invalidates the future (N4928 [futures.unique.future]/17)
_Retrieved = true;
_Maybe_run_deferred_function(_Lock);
while (!_Ready) {
Expand Down Expand Up @@ -361,7 +361,7 @@ public:
}

_STL_ASSERT(_Exc != nullptr, "promise<T>::set_exception/set_exception_at_thread_exit called with a null "
"std::exception_ptr, which is invalid per N4917 [futures.promise]/20");
"std::exception_ptr, which is invalid per N4928 [futures.promise]/20");
_Exception = _Exc;
_Do_notify(_Lock, _At_thread_exit);
}
Expand Down Expand Up @@ -879,7 +879,7 @@ private:

public:
static_assert(!is_array_v<_Ty> && is_object_v<_Ty> && is_destructible_v<_Ty>,
"T in future<T> must meet the Cpp17Destructible requirements (N4917 [futures.unique.future]/4).");
"T in future<T> must meet the Cpp17Destructible requirements (N4928 [futures.unique.future]/4).");

future() = default;

Expand Down Expand Up @@ -967,7 +967,7 @@ private:

public:
static_assert(!is_array_v<_Ty> && is_object_v<_Ty> && is_destructible_v<_Ty>,
"T in shared_future<T> must meet the Cpp17Destructible requirements (N4917 [futures.shared.future]/4).");
"T in shared_future<T> must meet the Cpp17Destructible requirements (N4928 [futures.shared.future]/4).");

shared_future() = default;

Expand Down Expand Up @@ -1043,27 +1043,21 @@ _NODISCARD inline shared_future<void> future<void>::share() noexcept {
template <class _Ty>
class _Promise {
public:
_Promise(_Associated_state<_Ty>* _State_ptr) : _State(_State_ptr, false), _Future_retrieved(false) {}
_Promise(_Associated_state<_Ty>* _State_ptr) noexcept : _State(_State_ptr, false), _Future_retrieved(false) {}

_Promise(_Promise&& _Other) : _State(_STD move(_Other._State)), _Future_retrieved(_Other._Future_retrieved) {}
_Promise(_Promise&&) = default;

@strega-nil-ms nicole mazzuca (strega-nil-ms) Jan 3, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note that this does not make _Promise<T> trivially moveable since _State_manager<T> is never trivially moveable (and therefore same for promise<T> and packaged_task<T>)

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) Jan 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, I think that makes this ABI-safe. (This is very close to the dangerous condition that I encountered in tuple<>'s copy ctor, though.)


_Promise& operator=(_Promise&& _Other) {
_State = _STD move(_Other._State);
_Future_retrieved = _Other._Future_retrieved;
return *this;
}
_Promise& operator=(_Promise&&) = default;

~_Promise() noexcept {}

void _Swap(_Promise& _Other) {
void _Swap(_Promise& _Other) noexcept {
_State._Swap(_Other._State);
_STD swap(_Future_retrieved, _Other._Future_retrieved);
}

const _State_manager<_Ty>& _Get_state() const {
const _State_manager<_Ty>& _Get_state() const noexcept {
return _State;
}
_State_manager<_Ty>& _Get_state() {
_State_manager<_Ty>& _Get_state() noexcept {
return _State;
}

Expand Down Expand Up @@ -1092,11 +1086,11 @@ public:
return _State.valid();
}

bool _Is_ready() const {
bool _Is_ready() const noexcept {
return _State._Is_ready();
}

bool _Is_ready_at_thread_exit() const {
bool _Is_ready_at_thread_exit() const noexcept {
return _State._Is_ready_at_thread_exit();
}

Expand All @@ -1112,14 +1106,14 @@ _EXPORT_STD template <class _Ty>
class promise { // class that defines an asynchronous provider that holds a value
public:
static_assert(!is_array_v<_Ty> && is_object_v<_Ty> && is_destructible_v<_Ty>,
"T in promise<T> must meet the Cpp17Destructible requirements (N4917 [futures.promise]/1).");
"T in promise<T> must meet the Cpp17Destructible requirements (N4928 [futures.promise]/1).");

promise() : _MyPromise(new _Associated_state<_Ty>) {}

template <class _Alloc>
promise(allocator_arg_t, const _Alloc& _Al) : _MyPromise(_Make_associated_state<_Ty>(_Al)) {}

promise(promise&& _Other) noexcept : _MyPromise(_STD move(_Other._MyPromise)) {}
promise(promise&&) = default;

promise& operator=(promise&& _Other) noexcept {
promise(_STD move(_Other)).swap(*this);
Expand Down Expand Up @@ -1181,7 +1175,7 @@ public:
template <class _Alloc>
promise(allocator_arg_t, const _Alloc& _Al) : _MyPromise(_Make_associated_state<_Ty*>(_Al)) {}

promise(promise&& _Other) noexcept : _MyPromise(_STD move(_Other._MyPromise)) {}
promise(promise&&) = default;

promise& operator=(promise&& _Other) noexcept {
promise(_STD move(_Other)).swap(*this);
Expand Down Expand Up @@ -1235,7 +1229,7 @@ public:
template <class _Alloc>
promise(allocator_arg_t, const _Alloc& _Al) : _MyPromise(_Make_associated_state<int>(_Al)) {}

promise(promise&& _Other) noexcept : _MyPromise(_STD move(_Other._MyPromise)) {}
promise(promise&&) = default;

promise& operator=(promise&& _Other) noexcept {
promise(_STD move(_Other)).swap(*this);
Expand Down Expand Up @@ -1316,17 +1310,14 @@ public:
using _MyStateManagerType = _State_manager<_Ptype>;
using _MyStateType = _Packaged_state<_Ret(_ArgTypes...)>;

packaged_task() noexcept : _MyPromise(0) {}
packaged_task() = default;
Comment on lines -1319 to +1313

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) Jan 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No change requested: Wow, a 0-as-null-pointer was still lurking in the codebase! Nice catch!


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))) {}

packaged_task(packaged_task&& _Other) noexcept : _MyPromise(_STD move(_Other._MyPromise)) {}
packaged_task(packaged_task&&) = default;

packaged_task& operator=(packaged_task&& _Other) noexcept {
_MyPromise = _STD move(_Other._MyPromise);
return *this;
}
packaged_task& operator=(packaged_task&&) = default;

#if _HAS_FUNCTION_ALLOCATOR_SUPPORT
template <class _Fty2, class _Alloc, enable_if_t<!is_same_v<_Remove_cvref_t<_Fty2>, packaged_task>, int> = 0>
Expand Down Expand Up @@ -1387,7 +1378,7 @@ public:
packaged_task& operator=(const packaged_task&) = delete;

private:
_MyPromiseType _MyPromise;
_MyPromiseType _MyPromise{nullptr};
};

#if _HAS_CXX17
Expand Down
34 changes: 34 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 @@ -283,6 +283,38 @@ void test_future_shared_future_noexcept() {
test_shared_future_noexcept_copy_impl<void>();
}

// Also test the exception specifications of move functions of promise and packaged_task
template <typename T>
void test_promise_noexcept_impl() {
STATIC_ASSERT(is_nothrow_move_constructible_v<promise<T>>);
STATIC_ASSERT(is_nothrow_move_assignable_v<promise<T>>);
STATIC_ASSERT(is_nothrow_destructible_v<promise<T>>);
}

void test_promise_noexcept() {
test_promise_noexcept_impl<int>();
test_promise_noexcept_impl<int&>();
test_promise_noexcept_impl<void>();
}

template <typename F>
void test_packaged_task_noexcept_impl() {
STATIC_ASSERT(is_nothrow_default_constructible_v<packaged_task<F>>);
STATIC_ASSERT(is_nothrow_move_constructible_v<packaged_task<F>>);
STATIC_ASSERT(is_nothrow_move_assignable_v<packaged_task<F>>);
STATIC_ASSERT(is_nothrow_destructible_v<packaged_task<F>>);
}

void test_packaged_task_noexcept() {
test_packaged_task_noexcept_impl<int()>();
test_packaged_task_noexcept_impl<int&()>();
test_packaged_task_noexcept_impl<void()>();

test_packaged_task_noexcept_impl<int(int)>();
test_packaged_task_noexcept_impl<int&(int)>();
test_packaged_task_noexcept_impl<void(int)>();
}

// Also test the non-constructibility of future from (future, {}) and (shared_future, {})
template <typename Void, typename T, typename... Args>
constexpr bool is_constructible_with_trailing_empty_brace_impl = false;
Expand Down Expand Up @@ -331,5 +363,7 @@ int main() {
test_VSO_115515();
test_VSO_272761();
test_future_shared_future_noexcept();
test_promise_noexcept();
test_packaged_task_noexcept();
test_no_implicit_brace_construction();
}