From 47e5e096637daed63fb7bc18de2072534401ad46 Mon Sep 17 00:00:00 2001 From: Mital Ashok Date: Mon, 14 Feb 2022 23:25:09 +0000 Subject: [PATCH 1/7] Allow promises to be constructed for non-default-constructible types --- stl/inc/future | 81 +++++++++++++++++++++------------ tests/tr1/tests/future/test.cpp | 4 +- 2 files changed, 55 insertions(+), 30 deletions(-) diff --git a/stl/inc/future b/stl/inc/future index 1eed675290f..125faa2c23d 100644 --- a/stl/inc/future +++ b/stl/inc/future @@ -184,23 +184,36 @@ struct _State_deleter : _Deleter_base<_Ty> { // manage allocator and deletion st _Alloc _My_alloc; }; +struct _DummyType {}; + +template +union _DummyUnionType { + _DummyType _Dummy{}; + _Ty _HeldValue; + operator _Ty&() noexcept { + return _HeldValue; + } +}; + template class _Associated_state { // class for managing associated synchronous state public: using _State_type = _Ty; using _Mydel = _Deleter_base<_Ty>; + // TRANSITION: _Associated_state ctor will try to default construct a _Ty _Associated_state(_Mydel* _Dp = nullptr) : _Refs(1), // non-atomic initialization _Exception(), _Retrieved(false), _Ready(false), _Ready_at_thread_exit(false), _Has_stored_result(false), - _Running(false), _Deleter(_Dp) { - // TRANSITION: _Associated_state ctor assumes _Ty is default constructible - } + _Running(false), _Deleter(_Dp) {} virtual ~_Associated_state() noexcept { - if (_Has_stored_result && !_Ready) { // registered for release at thread exit + if (_Already_has_stored_result() && !_Ready) { // registered for release at thread exit _Cond._Unregister(_Mtx); } + if (!is_default_constructible<_Ty>() && _Constructed_result()) { + static_cast<_Ty&>(_Result).~_Ty(); + } } void _Retain() { // increment reference count @@ -285,6 +298,24 @@ public: return _Result; } + template + void _Emplace_result_2(_Ty2&& _Val, true_type) { + _Result = _STD forward<_Ty2>(_Val); + } + + template + void _Emplace_result_2(_Ty2&& _Val, false_type) { + ::new (static_cast(_STD addressof(_Result._HeldValue))) _Ty(_STD forward<_Ty2>(_Val)); + _Has_stored_result = true; + } + + template + void _Emplace_result(_Ty2&& _Val) { + // TRANSITION: _Associated_state ctor will move or copy assign if _Result was default constructed + // rather than move or copy construct + _Emplace_result_2(_STD forward<_Ty2>(_Val), is_default_constructible<_Ty>{}); + } + void _Set_value(const _Ty& _Val, bool _At_thread_exit) { // store a result unique_lock _Lock(_Mtx); _Set_value_raw(_Val, &_Lock, _At_thread_exit); @@ -292,11 +323,11 @@ public: void _Set_value_raw(const _Ty& _Val, unique_lock* _Lock, bool _At_thread_exit) { // store a result while inside a locked block - if (_Has_stored_result) { + if (_Already_has_stored_result()) { _Throw_future_error(make_error_code(future_errc::promise_already_satisfied)); } - _Result = _Val; + _Emplace_result(_Val); _Do_notify(_Lock, _At_thread_exit); } @@ -307,25 +338,11 @@ public: void _Set_value_raw(_Ty&& _Val, unique_lock* _Lock, bool _At_thread_exit) { // store a result while inside a locked block - if (_Has_stored_result) { - _Throw_future_error(make_error_code(future_errc::promise_already_satisfied)); - } - - _Result = _STD forward<_Ty>(_Val); - _Do_notify(_Lock, _At_thread_exit); - } - - void _Set_value(bool _At_thread_exit) { // store a (void) result - unique_lock _Lock(_Mtx); - _Set_value_raw(&_Lock, _At_thread_exit); - } - - void _Set_value_raw( - unique_lock* _Lock, bool _At_thread_exit) { // store a (void) result while inside a locked block - if (_Has_stored_result) { + if (_Already_has_stored_result()) { _Throw_future_error(make_error_code(future_errc::promise_already_satisfied)); } + _Emplace_result(_STD forward<_Ty>(_Val)); _Do_notify(_Lock, _At_thread_exit); } @@ -336,10 +353,11 @@ public: void _Set_exception_raw(exception_ptr _Exc, unique_lock* _Lock, bool _At_thread_exit) { // store a result while inside a locked block - if (_Has_stored_result) { + if (_Already_has_stored_result()) { _Throw_future_error(make_error_code(future_errc::promise_already_satisfied)); } + _STL_ASSERT((_Exc != nullptr), "promise::set_exception(nullptr)"); _Exception = _Exc; _Do_notify(_Lock, _At_thread_exit); } @@ -352,8 +370,13 @@ public: return _Ready_at_thread_exit; } - bool _Already_has_stored_result() const { - return _Has_stored_result; + bool _Already_has_stored_result() const { // Has a result or exception + return is_default_constructible<_Ty>::value ? _Has_stored_result + : (_Has_stored_result || _Exception != nullptr); + } + + bool _Constructed_result() const { + return is_default_constructible<_Ty>::value || _Has_stored_result; } bool _Already_retrieved() const { @@ -362,7 +385,7 @@ public: void _Abandon() { // abandon shared state unique_lock _Lock(_Mtx); - if (!_Has_stored_result) { // queue exception + if (!_Already_has_stored_result()) { // queue exception future_error _Fut(make_error_code(future_errc::broken_promise)); _Set_exception_raw(_STD make_exception_ptr(_Fut), &_Lock, false); } @@ -383,7 +406,7 @@ protected: } public: - _Ty _Result; + typename conditional::value, _Ty, _DummyUnionType<_Ty>>::type _Result; exception_ptr _Exception; mutex _Mtx; condition_variable _Cond; @@ -402,7 +425,9 @@ private: virtual void _Do_notify(unique_lock* _Lock, bool _At_thread_exit) { // notify waiting threads // TRANSITION, ABI: This is virtual, but never overridden. - _Has_stored_result = true; + if (is_default_constructible<_Ty>::value) { + _Has_stored_result = true; + } if (_At_thread_exit) { // notify at thread exit _Cond._Register(*_Lock, &_Ready); } else { // notify immediately diff --git a/tests/tr1/tests/future/test.cpp b/tests/tr1/tests/future/test.cpp index 08a0f3fb5d5..1c8ca7b7299 100644 --- a/tests/tr1/tests/future/test.cpp +++ b/tests/tr1/tests/future/test.cpp @@ -268,13 +268,13 @@ static void call_promise_setter(STD promise* pr, int which) { // try to set pr->set_value(3); break; case 1: - pr->set_exception(STD exception_ptr()); + pr->set_exception(STD make_exception_ptr(0)); break; case 2: pr->set_value_at_thread_exit(3); break; case 3: - pr->set_exception_at_thread_exit(STD exception_ptr()); + pr->set_exception_at_thread_exit(STD make_exception_ptr(0)); break; } } From 507136b8a85949bc2ee406b7a80ace224de79c6f Mon Sep 17 00:00:00 2001 From: Mital Ashok Date: Tue, 15 Feb 2022 14:19:41 +0000 Subject: [PATCH 2/7] Use if constexpr --- stl/inc/future | 41 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/stl/inc/future b/stl/inc/future index 125faa2c23d..5420ee9ff2b 100644 --- a/stl/inc/future +++ b/stl/inc/future @@ -211,8 +211,10 @@ public: if (_Already_has_stored_result() && !_Ready) { // registered for release at thread exit _Cond._Unregister(_Mtx); } - if (!is_default_constructible<_Ty>() && _Constructed_result()) { - static_cast<_Ty&>(_Result).~_Ty(); + if constexpr (!is_default_constructible_v<_Ty>) { + if (_Has_stored_result) { + _Result._HeldValue.~Ty(); + } } } @@ -298,22 +300,16 @@ public: return _Result; } - template - void _Emplace_result_2(_Ty2&& _Val, true_type) { - _Result = _STD forward<_Ty2>(_Val); - } - - template - void _Emplace_result_2(_Ty2&& _Val, false_type) { - ::new (static_cast(_STD addressof(_Result._HeldValue))) _Ty(_STD forward<_Ty2>(_Val)); - _Has_stored_result = true; - } - template void _Emplace_result(_Ty2&& _Val) { // TRANSITION: _Associated_state ctor will move or copy assign if _Result was default constructed // rather than move or copy construct - _Emplace_result_2(_STD forward<_Ty2>(_Val), is_default_constructible<_Ty>{}); + if constexpr (is_default_constructible_v<_Ty>) { + _Result = _STD forward<_Ty2>(_Val); + } else { + ::new (static_cast(_STD addressof(_Result._HeldValue))) _Ty(_STD forward<_Ty2>(_Val)); + _Has_stored_result = true; + } } void _Set_value(const _Ty& _Val, bool _At_thread_exit) { // store a result @@ -370,13 +366,12 @@ public: return _Ready_at_thread_exit; } - bool _Already_has_stored_result() const { // Has a result or exception - return is_default_constructible<_Ty>::value ? _Has_stored_result - : (_Has_stored_result || _Exception != nullptr); - } - - bool _Constructed_result() const { - return is_default_constructible<_Ty>::value || _Has_stored_result; + bool _Already_has_stored_result() const { // Has a result or an exception + if constexpr (is_default_constructible_v<_Ty>) { + return _Has_stored_result; + } else { + return _Has_stored_result || (_Exception != nullptr); + } } bool _Already_retrieved() const { @@ -406,7 +401,7 @@ protected: } public: - typename conditional::value, _Ty, _DummyUnionType<_Ty>>::type _Result; + typename conditional, _Ty, _DummyUnionType<_Ty>>::type _Result; exception_ptr _Exception; mutex _Mtx; condition_variable _Cond; @@ -425,7 +420,7 @@ private: virtual void _Do_notify(unique_lock* _Lock, bool _At_thread_exit) { // notify waiting threads // TRANSITION, ABI: This is virtual, but never overridden. - if (is_default_constructible<_Ty>::value) { + if constexpr (is_default_constructible_v<_Ty>) { _Has_stored_result = true; } if (_At_thread_exit) { // notify at thread exit From 6c86ccd662af455ce8b84980bd395a670d3f0c65 Mon Sep 17 00:00:00 2001 From: Mital Ashok Date: Wed, 16 Feb 2022 07:15:19 +0000 Subject: [PATCH 3/7] Style changes --- stl/inc/future | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/stl/inc/future b/stl/inc/future index 5420ee9ff2b..c457522bfac 100644 --- a/stl/inc/future +++ b/stl/inc/future @@ -184,14 +184,14 @@ struct _State_deleter : _Deleter_base<_Ty> { // manage allocator and deletion st _Alloc _My_alloc; }; -struct _DummyType {}; +template +union _Result_holder { + _Result_holder() {} + ~_Result_holder() {} -template -union _DummyUnionType { - _DummyType _Dummy{}; - _Ty _HeldValue; + _Ty _Held_value; operator _Ty&() noexcept { - return _HeldValue; + return _Held_value; } }; @@ -211,9 +211,10 @@ public: if (_Already_has_stored_result() && !_Ready) { // registered for release at thread exit _Cond._Unregister(_Mtx); } + if constexpr (!is_default_constructible_v<_Ty>) { if (_Has_stored_result) { - _Result._HeldValue.~Ty(); + _Result._Held_value.~_Ty(); } } } @@ -297,17 +298,21 @@ public: _Rethrow_future_exception(_Exception); } - return _Result; + if constexpr (is_default_constructible_v<_Ty>) { + return _Result; + } else { + return _Result._Held_value; + } } - template + template void _Emplace_result(_Ty2&& _Val) { // TRANSITION: _Associated_state ctor will move or copy assign if _Result was default constructed // rather than move or copy construct if constexpr (is_default_constructible_v<_Ty>) { _Result = _STD forward<_Ty2>(_Val); } else { - ::new (static_cast(_STD addressof(_Result._HeldValue))) _Ty(_STD forward<_Ty2>(_Val)); + ::new (static_cast(_STD addressof(_Result._Held_value))) _Ty(_STD forward<_Ty2>(_Val)); _Has_stored_result = true; } } @@ -353,7 +358,8 @@ public: _Throw_future_error(make_error_code(future_errc::promise_already_satisfied)); } - _STL_ASSERT((_Exc != nullptr), "promise::set_exception(nullptr)"); + _STL_ASSERT(_Exc != nullptr, "promise::set_exception/set_exception_at_thread_exit called with a null " + "std::exception_ptr, which is invalid per N4901 32.9.6 [future.promise]/20"); _Exception = _Exc; _Do_notify(_Lock, _At_thread_exit); } @@ -370,7 +376,7 @@ public: if constexpr (is_default_constructible_v<_Ty>) { return _Has_stored_result; } else { - return _Has_stored_result || (_Exception != nullptr); + return _Has_stored_result || _Exception; } } @@ -401,7 +407,7 @@ protected: } public: - typename conditional, _Ty, _DummyUnionType<_Ty>>::type _Result; + conditional_t, _Ty, _Result_holder<_Ty>> _Result; exception_ptr _Exception; mutex _Mtx; condition_variable _Cond; @@ -423,6 +429,7 @@ private: if constexpr (is_default_constructible_v<_Ty>) { _Has_stored_result = true; } + if (_At_thread_exit) { // notify at thread exit _Cond._Register(*_Lock, &_Ready); } else { // notify immediately From cbf9965106c13ec3370defd7c9729eedc484be89 Mon Sep 17 00:00:00 2001 From: Mital Ashok Date: Wed, 16 Feb 2022 07:18:32 +0000 Subject: [PATCH 4/7] Created test file --- tests/std/test.lst | 1 + .../env.lst | 4 + .../test.cpp | 199 ++++++++++++++++++ 3 files changed, 204 insertions(+) create mode 100644 tests/std/tests/GH_002488_promise_not_default_constructible_types/env.lst create mode 100644 tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp diff --git a/tests/std/test.lst b/tests/std/test.lst index af4d747a46e..8379ab03809 100644 --- a/tests/std/test.lst +++ b/tests/std/test.lst @@ -191,6 +191,7 @@ tests\GH_002030_asan_annotate_vector tests\GH_002039_byte_is_not_trivially_swappable tests\GH_002058_debug_iterator_race tests\GH_002120_streambuf_seekpos_and_seekoff +tests\GH_002488_promise_not_default_constructible_types tests\LWG2597_complex_branch_cut tests\LWG3018_shared_ptr_function tests\LWG3146_excessive_unwrapping_ref_cref diff --git a/tests/std/tests/GH_002488_promise_not_default_constructible_types/env.lst b/tests/std/tests/GH_002488_promise_not_default_constructible_types/env.lst new file mode 100644 index 00000000000..e970fe46ae6 --- /dev/null +++ b/tests/std/tests/GH_002488_promise_not_default_constructible_types/env.lst @@ -0,0 +1,4 @@ +# Copyright (c) Microsoft Corporation. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +RUNALL_INCLUDE ..\native_matrix.lst diff --git a/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp b/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp new file mode 100644 index 00000000000..c2cfbcc2a6b --- /dev/null +++ b/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp @@ -0,0 +1,199 @@ +// Copyright (c) Microsoft Corporation. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + + +#include +#include +#include + + +static std::atomic has_default_objects{0}; +static std::atomic no_default_objects{0}; +static std::atomic no_default_or_assign_objects{0}; + + +struct has_default { + has_default() : x(0xbad) { + ++has_default_objects; + } + explicit has_default(int n) : x(n) { + ++has_default_objects; + } + has_default(const has_default& v) : x(v.x) { + ++has_default_objects; + } + + ~has_default() { + --has_default_objects; + } + + int x; +}; + +struct no_default { + no_default() = delete; + explicit no_default(int n) : x(n) { + ++no_default_objects; + } + no_default(const no_default& v) : x(v.x) { + ++no_default_objects; + } + + ~no_default() { + --no_default_objects; + } + + int x; +}; + +struct no_default_or_assign { + no_default_or_assign() = delete; + explicit no_default_or_assign(int n) : x(n) { + ++no_default_or_assign_objects; + } + no_default_or_assign(const no_default_or_assign& v) : x(v.x) { + ++no_default_or_assign_objects; + } + + void operator=(const no_default_or_assign&) = delete; + + ~no_default_or_assign() { + --no_default_or_assign_objects; + } + + int x; +}; + +template +void assert_throws_future_error(F f, std::error_code errc) { + try { + f(); + } catch (const std::future_error& e) { + assert(e.code() == errc); + return; + } catch (...) {} + assert(false); +} + +template +void run_tests() { + using promise = std::promise; + using future = std::future; + + { + promise p; + p.set_value(T(4)); + assert(p.get_future().get().x == 4); + } + + { + promise p; + future f = p.get_future(); + T v(10); + p.set_value(v); + assert(f.get().x == 10); + assert_throws_future_error([&]{ + p.set_value(v); + }, std::future_errc::promise_already_satisfied); + assert_throws_future_error([&]{ + f.get(); + }, std::future_errc::no_state); + assert_throws_future_error([&]{ + p.get_future().get(); + }, std::future_errc::future_already_retrieved); + } + + { + promise p; + future f = p.get_future(); + p.set_exception(std::make_exception_ptr(5)); + try { + f.get(); + assert(false); + } catch (int i) { + assert(i == 5); + } catch (...) { + assert(false); + } + } + + { + promise p; + future f = p.get_future(); + p.set_exception(std::make_exception_ptr(3)); + assert_throws_future_error([&]{ + p.set_value(T(2)); + }, std::future_errc::promise_already_satisfied); + try { + f.get(); + assert(false); + } catch (int i) { + assert(i == 3); + } catch (...) { + assert(false); + } + } + + { + promise p; + future f = p.get_future(); + std::atomic failures{0}; + int succeeded = -1; + auto make_thread = [&](int n) { + return std::thread([&, n]{ + try { + p.set_value(T(n)); + } catch (std::future_error) { + ++failures; + return; + } + succeeded = n; + }); + }; + std::thread threads[]{ + make_thread(0), make_thread(1), make_thread(2), make_thread(3), + make_thread(4), make_thread(5), make_thread(6), make_thread(7) + }; + + for (auto& t : threads) { + t.join(); + } + + assert(failures == 7); + assert(succeeded != -1 && f.get().x == succeeded); + } + + { + (void) std::async(std::launch::async, []() -> T { + return T(16); + }); + (void) std::async(std::launch::async, []() -> T { + const T x(40); + return x; + }); + + future f = std::async(std::launch::async, []() -> T { + return T(23); + }); + assert(f.get().x == 23); + } + + { + std::packaged_task pt([]() -> T { + return T(7); + }); + future f = pt.get_future(); + pt(); + + assert(f.get().x == 7); + } +} + +int main() { + run_tests(); + run_tests(); + run_tests(); + assert(has_default_objects == 0); + assert(no_default_objects == 0); + assert(no_default_or_assign_objects == 0); +} From d8b03e29e67ff99e891118276a6abc35657ed678 Mon Sep 17 00:00:00 2001 From: Mital Ashok Date: Wed, 16 Feb 2022 07:30:48 +0000 Subject: [PATCH 5/7] clang-tidy test --- .../test.cpp | 47 +++++++------------ 1 file changed, 16 insertions(+), 31 deletions(-) diff --git a/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp b/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp index c2cfbcc2a6b..3dfcda4385c 100644 --- a/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp +++ b/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp @@ -3,8 +3,8 @@ #include -#include #include +#include static std::atomic has_default_objects{0}; @@ -64,21 +64,22 @@ struct no_default_or_assign { int x; }; -template +template void assert_throws_future_error(F f, std::error_code errc) { try { f(); } catch (const std::future_error& e) { assert(e.code() == errc); return; - } catch (...) {} + } catch (...) { + } assert(false); } template void run_tests() { using promise = std::promise; - using future = std::future; + using future = std::future; { promise p; @@ -92,15 +93,9 @@ void run_tests() { T v(10); p.set_value(v); assert(f.get().x == 10); - assert_throws_future_error([&]{ - p.set_value(v); - }, std::future_errc::promise_already_satisfied); - assert_throws_future_error([&]{ - f.get(); - }, std::future_errc::no_state); - assert_throws_future_error([&]{ - p.get_future().get(); - }, std::future_errc::future_already_retrieved); + assert_throws_future_error([&] { p.set_value(v); }, std::future_errc::promise_already_satisfied); + assert_throws_future_error([&] { f.get(); }, std::future_errc::no_state); + assert_throws_future_error([&] { p.get_future().get(); }, std::future_errc::future_already_retrieved); } { @@ -121,9 +116,7 @@ void run_tests() { promise p; future f = p.get_future(); p.set_exception(std::make_exception_ptr(3)); - assert_throws_future_error([&]{ - p.set_value(T(2)); - }, std::future_errc::promise_already_satisfied); + assert_throws_future_error([&] { p.set_value(T(2)); }, std::future_errc::promise_already_satisfied); try { f.get(); assert(false); @@ -138,9 +131,9 @@ void run_tests() { promise p; future f = p.get_future(); std::atomic failures{0}; - int succeeded = -1; + int succeeded = -1; auto make_thread = [&](int n) { - return std::thread([&, n]{ + return std::thread([&, n] { try { p.set_value(T(n)); } catch (std::future_error) { @@ -150,10 +143,8 @@ void run_tests() { succeeded = n; }); }; - std::thread threads[]{ - make_thread(0), make_thread(1), make_thread(2), make_thread(3), - make_thread(4), make_thread(5), make_thread(6), make_thread(7) - }; + std::thread threads[]{make_thread(0), make_thread(1), make_thread(2), make_thread(3), make_thread(4), + make_thread(5), make_thread(6), make_thread(7)}; for (auto& t : threads) { t.join(); @@ -164,24 +155,18 @@ void run_tests() { } { - (void) std::async(std::launch::async, []() -> T { - return T(16); - }); + (void) std::async(std::launch::async, []() -> T { return T(16); }); (void) std::async(std::launch::async, []() -> T { const T x(40); return x; }); - future f = std::async(std::launch::async, []() -> T { - return T(23); - }); + future f = std::async(std::launch::async, []() -> T { return T(23); }); assert(f.get().x == 23); } { - std::packaged_task pt([]() -> T { - return T(7); - }); + std::packaged_task pt([]() -> T { return T(7); }); future f = pt.get_future(); pt(); From 21416b76dc514874593c9c615ed411dce13d1fc8 Mon Sep 17 00:00:00 2001 From: Mital Ashok Date: Thu, 17 Feb 2022 04:31:51 +0000 Subject: [PATCH 6/7] Test naming / include fixes --- stl/inc/future | 4 +- .../test.cpp | 39 ++++++++++--------- 2 files changed, 24 insertions(+), 19 deletions(-) diff --git a/stl/inc/future b/stl/inc/future index c457522bfac..6ea467dc7c3 100644 --- a/stl/inc/future +++ b/stl/inc/future @@ -288,6 +288,8 @@ public: _Rethrow_future_exception(_Exception); } + // TRANSITION: This should be assigned before `_Exception` is rethrown so that a `future::get` + // that throws a stored exception is invalidated (see: N4901 [futures.unique.future]/17) _Retrieved = true; _Maybe_run_deferred_function(_Lock); while (!_Ready) { @@ -359,7 +361,7 @@ public: } _STL_ASSERT(_Exc != nullptr, "promise::set_exception/set_exception_at_thread_exit called with a null " - "std::exception_ptr, which is invalid per N4901 32.9.6 [future.promise]/20"); + "std::exception_ptr, which is invalid per N4901 32.9.6 [futures.promise]/20"); _Exception = _Exc; _Do_notify(_Lock, _At_thread_exit); } diff --git a/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp b/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp index 3dfcda4385c..737ea855fb2 100644 --- a/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp +++ b/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp @@ -2,8 +2,11 @@ // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +#include #include +#include #include +#include #include @@ -65,11 +68,11 @@ struct no_default_or_assign { }; template -void assert_throws_future_error(F f, std::error_code errc) { +void assert_throws_future_error(F f, std::error_code expected_code) { try { f(); } catch (const std::future_error& e) { - assert(e.code() == errc); + assert(e.code() == expected_code); return; } catch (...) { } @@ -78,18 +81,18 @@ void assert_throws_future_error(F f, std::error_code errc) { template void run_tests() { - using promise = std::promise; - using future = std::future; + using Promise = std::promise; + using Future = std::future; { - promise p; + Promise p; p.set_value(T(4)); assert(p.get_future().get().x == 4); } { - promise p; - future f = p.get_future(); + Promise p; + Future f = p.get_future(); T v(10); p.set_value(v); assert(f.get().x == 10); @@ -99,8 +102,8 @@ void run_tests() { } { - promise p; - future f = p.get_future(); + Promise p; + Future f = p.get_future(); p.set_exception(std::make_exception_ptr(5)); try { f.get(); @@ -113,8 +116,8 @@ void run_tests() { } { - promise p; - future f = p.get_future(); + Promise p; + Future f = p.get_future(); p.set_exception(std::make_exception_ptr(3)); assert_throws_future_error([&] { p.set_value(T(2)); }, std::future_errc::promise_already_satisfied); try { @@ -128,8 +131,8 @@ void run_tests() { } { - promise p; - future f = p.get_future(); + Promise p; + Future f = p.get_future(); std::atomic failures{0}; int succeeded = -1; auto make_thread = [&](int n) { @@ -155,19 +158,19 @@ void run_tests() { } { - (void) std::async(std::launch::async, []() -> T { return T(16); }); - (void) std::async(std::launch::async, []() -> T { + (void) std::async(std::launch::async, [] { return T(16); }); + (void) std::async(std::launch::async, [] { const T x(40); return x; }); - future f = std::async(std::launch::async, []() -> T { return T(23); }); + Future f = std::async(std::launch::async, [] { return T(23); }); assert(f.get().x == 23); } { - std::packaged_task pt([]() -> T { return T(7); }); - future f = pt.get_future(); + std::packaged_task pt([] { return T(7); }); + Future f = pt.get_future(); pt(); assert(f.get().x == 7); From 1274a5b0ae0864b2dea50c7d4ea36124ccb78b0d Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Mon, 28 Feb 2022 18:20:19 -0800 Subject: [PATCH 7/7] Code review feedback. --- stl/inc/future | 12 ++++-------- .../test.cpp | 3 --- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/stl/inc/future b/stl/inc/future index 6ea467dc7c3..66e2eb0f4ca 100644 --- a/stl/inc/future +++ b/stl/inc/future @@ -190,9 +190,6 @@ union _Result_holder { ~_Result_holder() {} _Ty _Held_value; - operator _Ty&() noexcept { - return _Held_value; - } }; template @@ -201,7 +198,7 @@ public: using _State_type = _Ty; using _Mydel = _Deleter_base<_Ty>; - // TRANSITION: _Associated_state ctor will try to default construct a _Ty + // TRANSITION, incorrectly default constructs _Result when _Ty is default constructible _Associated_state(_Mydel* _Dp = nullptr) : _Refs(1), // non-atomic initialization _Exception(), _Retrieved(false), _Ready(false), _Ready_at_thread_exit(false), _Has_stored_result(false), @@ -288,8 +285,8 @@ public: _Rethrow_future_exception(_Exception); } - // TRANSITION: This should be assigned before `_Exception` is rethrown so that a `future::get` - // that throws a stored exception is invalidated (see: N4901 [futures.unique.future]/17) + // TRANSITION: `_Retrieved` should be assigned before `_Exception` is thrown so that a `future::get` + // that throws a stored exception invalidates the future (N4901 [futures.unique.future]/17) _Retrieved = true; _Maybe_run_deferred_function(_Lock); while (!_Ready) { @@ -309,8 +306,7 @@ public: template void _Emplace_result(_Ty2&& _Val) { - // TRANSITION: _Associated_state ctor will move or copy assign if _Result was default constructed - // rather than move or copy construct + // TRANSITION, incorrectly assigns _Result when _Ty is default constructible if constexpr (is_default_constructible_v<_Ty>) { _Result = _STD forward<_Ty2>(_Val); } else { diff --git a/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp b/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp index 737ea855fb2..0f4dedb10a0 100644 --- a/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp +++ b/tests/std/tests/GH_002488_promise_not_default_constructible_types/test.cpp @@ -1,7 +1,6 @@ // Copyright (c) Microsoft Corporation. // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception - #include #include #include @@ -9,12 +8,10 @@ #include #include - static std::atomic has_default_objects{0}; static std::atomic no_default_objects{0}; static std::atomic no_default_or_assign_objects{0}; - struct has_default { has_default() : x(0xbad) { ++has_default_objects;