From 95e91f333b2867778747154d79ded0c69f57824e Mon Sep 17 00:00:00 2001 From: Dix Lorenz Date: Tue, 3 Jul 2018 10:36:26 +0200 Subject: [PATCH 1/4] comparison functions check for identity first --- stlab/copy_on_write.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/stlab/copy_on_write.hpp b/stlab/copy_on_write.hpp index 186bd829d..18f7c3a67 100644 --- a/stlab/copy_on_write.hpp +++ b/stlab/copy_on_write.hpp @@ -134,7 +134,7 @@ class copy_on_write { } friend inline bool operator<(const copy_on_write& x, const copy_on_write& y) noexcept { - return *x < *y; + return !x.identity(y) && (*x < *y); } friend inline bool operator<(const copy_on_write& x, const element_type& y) noexcept { @@ -182,7 +182,7 @@ class copy_on_write { } friend inline bool operator==(const copy_on_write& x, const copy_on_write& y) noexcept { - return *x == *y; + return x.identity(y) || (*x == *y); } friend inline bool operator==(const copy_on_write& x, const element_type& y) noexcept { From d812263635f966e153e15ec9a037ec4b209bfd06 Mon Sep 17 00:00:00 2001 From: Felix Petriconi Date: Sat, 28 Jul 2018 15:23:39 +0200 Subject: [PATCH 2/4] Correcting execution of ready when_all/when_any on the correct executor --- stlab/concurrency/future.hpp | 148 +++++++++++------------ test/future_test_helper.hpp | 5 +- test/future_when_all_arguments_tests.cpp | 40 ++++++ test/future_when_any_range_tests.cpp | 2 +- 4 files changed, 112 insertions(+), 83 deletions(-) diff --git a/stlab/concurrency/future.hpp b/stlab/concurrency/future.hpp index 427c4ecf0..c8c35c685 100644 --- a/stlab/concurrency/future.hpp +++ b/stlab/concurrency/future.hpp @@ -217,8 +217,8 @@ struct value_setter; /**************************************************************************************************/ -template -auto package(S&&, F &&) +template +auto package(E&&, F &&) -> std::pair, future>>; /**************************************************************************************************/ @@ -273,9 +273,9 @@ struct shared_base> : std::enable_shared_from_this - auto then(S&& s, F&& f) { - return recover(std::forward(s), + template + auto then(E&& executor, F&& f) { + return recover(std::forward(executor), [_f = std::forward(f)](const auto& x) { return _f(x._p->get_ready()); }); } @@ -284,10 +284,10 @@ struct shared_base> : std::enable_shared_from_this(f)); } - template - auto recover(S s, F&& f) { + template + auto recover(E executor, F&& f) { auto p = package)>()>( - s, [_f = std::forward(f), _p = future(this->shared_from_this())]() mutable { + executor, [_f = std::forward(f), _p = future(this->shared_from_this())]() mutable { return std::move(_f)(std::move(_p)); }); @@ -295,9 +295,9 @@ struct shared_base> : std::enable_shared_from_this lock(_mutex); ready = _ready; - if (!ready) _then.emplace_back(std::move(s), std::move(p.first)); + if (!ready) _then.emplace_back(std::move(executor), std::move(p.first)); } - if (ready) s(std::move(p.first)); + if (ready) executor(std::move(p.first)); return reduce(std::move(p.second)); } @@ -307,9 +307,9 @@ struct shared_base> : std::enable_shared_from_this(f)); } - template - auto then_r(bool unique, S&& s, F&& f) { - return recover_r(unique, std::forward(s), [_f = std::forward(f)](auto&& x) mutable { + template + auto then_r(bool unique, E&& executor, F&& f) { + return recover_r(unique, std::forward(executor), [_f = std::forward(f)](auto&& x) mutable { return _f(std::move(*(std::forward(x).get_try()))); }); } @@ -319,12 +319,12 @@ struct shared_base> : std::enable_shared_from_this(f)); } - template - auto recover_r(bool unique, S&& s, F&& f) { - if (!unique) return recover(std::forward(s), std::forward(f)); + template + auto recover_r(bool unique, E&& executor, F&& f) { + if (!unique) return recover(std::forward(executor), std::forward(f)); auto p = package)>()>( - s, [_f = std::forward(f), _p = future(this->shared_from_this())]() mutable { + executor, [_f = std::forward(f), _p = future(this->shared_from_this())]() mutable { return _f(std::move(_p)); }); @@ -332,9 +332,9 @@ struct shared_base> : std::enable_shared_from_this lock(_mutex); ready = _ready; - if (!ready) _then.emplace_back(std::move(s), std::move(p.first)); + if (!ready) _then.emplace_back(std::move(executor), std::move(p.first)); } - if (ready) s(std::move(p.first)); + if (ready) executor(std::move(p.first)); return reduce(std::move(p.second)); } @@ -444,11 +444,11 @@ struct shared_base> : std::enable_shared_from_this< return recover_r(unique, _executor, std::forward(f)); } - template - auto recover_r(bool, S s, F&& f) { + template + auto recover_r(bool, E executor, F&& f) { // rvalue case unique is assumed. auto p = package)>()>( - s, [_f = std::forward(f), _p = future(this->shared_from_this())]() mutable { + executor, [_f = std::forward(f), _p = future(this->shared_from_this())]() mutable { return _f(std::move(_p)); }); @@ -456,9 +456,9 @@ struct shared_base> : std::enable_shared_from_this< { std::unique_lock lock(_mutex); ready = _ready; - if (!ready) _then = {std::move(s), std::move(p.first)}; + if (!ready) _then = {std::move(executor), std::move(p.first)}; } - if (ready) s(std::move(p.first)); + if (ready) executor(std::move(p.first)); return reduce(std::move(p.second)); } @@ -663,12 +663,12 @@ class packaged_task { explicit packaged_task(ptr_t p) : _p(std::move(p)) {} - template - friend auto package(S&&, F &&) -> std::pair, + template + friend auto package(E&&, F &&) -> std::pair, future>>; - template - friend auto package_with_broken_promise(S&&, F &&) + template + friend auto package_with_broken_promise(E&&, F&&) -> std::pair, future>>; @@ -714,12 +714,12 @@ class future> { explicit future(ptr_t p) : _p(std::move(p)) {} - template - friend auto package(S&&, F &&) -> std::pair, + template + friend auto package(E&&, F&&) -> std::pair, future>>; - template - friend auto package_with_broken_promise(S&&, F &&) + template + friend auto package_with_broken_promise(E&&, F &&) -> std::pair, future>>; @@ -805,12 +805,12 @@ class future { explicit future(ptr_t p) : _p(std::move(p)) {} - template - friend auto package(S&&, F &&) -> std::pair, + template + friend auto package(E&&, F &&) -> std::pair, future>>; - template - friend auto package_with_broken_promise(S&&, F &&) + template + friend auto package_with_broken_promise(E&&, F &&) -> std::pair, future>>; @@ -895,12 +895,12 @@ class future> { explicit future(ptr_t p) : _p(std::move(p)) {} future(const future&) = default; - template - friend auto package(S&&, F &&) -> std::pair, - future>>; + template + friend auto package(E&&, F &&) -> std::pair, + future>>; - template - friend auto package_with_broken_promise(S&&, F &&) + template + friend auto package_with_broken_promise(E&&, F &&) -> std::pair, future>>; @@ -960,18 +960,18 @@ class future> { stlab::optional error() const& { return _p->_error; } }; -template -auto package(S&& s, F&& f) +template +auto package(E&& executor, F&& f) -> std::pair, future>> { - auto p = std::make_shared>(std::forward(s), std::forward(f)); + auto p = std::make_shared>(std::forward(executor), std::forward(f)); return std::make_pair(detail::packaged_task_from_signature_t(p), future>(p)); } -template -auto package_with_broken_promise(S&& s, F&& f) +template +auto package_with_broken_promise(E&& executor, F&& f) -> std::pair, future>> { - auto p = std::make_shared>(std::forward(s), std::forward(f)); + auto p = std::make_shared>(std::forward(executor), std::forward(f)); auto result = std::make_pair(detail::packaged_task_from_signature_t(p), future>(p)); result.second._p->_error = @@ -1124,9 +1124,9 @@ auto apply_when_any_arg(F& f, P& p) { return p->apply(f); } -template -void attach_when_arg_(const std::shared_ptr

& p, T a) { - p->_holds[i] = std::move(a).recover([_w = std::weak_ptr

(p)](auto x) { +template +void attach_when_arg_(E&& executor, const std::shared_ptr

& p, T a) { + p->_holds[i] = std::move(a).recover(std::forward(executor), [_w = std::weak_ptr

(p)](auto x) { auto p = _w.lock(); if (!p) return; @@ -1139,14 +1139,14 @@ void attach_when_arg_(const std::shared_ptr

& p, T a) { }); } -template -void attach_when_args_(std::index_sequence, const std::shared_ptr

& p, Ts... a) { - (void)std::initializer_list{(attach_when_arg_(p, a), 0)...}; +template +void attach_when_args_(std::index_sequence, E&& executor, const std::shared_ptr

& p, Ts... a) { + (void)std::initializer_list{(attach_when_arg_(std::forward(executor), p, a), 0)...}; } -template -void attach_when_args(const std::shared_ptr

& p, Ts... a) { - attach_when_args_(std::make_index_sequence(), p, std::move(a)...); +template +void attach_when_args(E&& executor, const std::shared_ptr

& p, Ts... a) { + attach_when_args_(std::make_index_sequence(), std::forward(executor), p, std::move(a)...); } } // namespace detail @@ -1165,9 +1165,7 @@ auto when_all(E executor, F f, future... args) { }); shared->_f = std::move(p.first); - detail::attach_when_args(shared, std::move(args)...); - - executor(std::move(p.first)); + detail::attach_when_args(std::move(executor), shared, std::move(args)...); return std::move(p.second); } @@ -1186,9 +1184,7 @@ struct make_when_any { }); shared->_f = std::move(p.first); - detail::attach_when_args(shared, std::move(arg), std::move(args)...); - - executor(std::move(p.first)); + detail::attach_when_args(std::move(executor), shared, std::move(arg), std::move(args)...); return std::move(p.second); } @@ -1208,9 +1204,7 @@ struct make_when_any { }); shared->_f = std::move(p.first); - detail::attach_when_args(shared, std::move(args)...); - - executor(std::move(p.first)); + detail::attach_when_args(std::move(executor), shared, std::move(args)...); return std::move(p.second); } @@ -1397,10 +1391,10 @@ struct common_context : CR { /**************************************************************************************************/ -template -void attach_tasks(size_t index, const std::shared_ptr& context, T&& a) { +template +void attach_tasks(size_t index, E&& executor, const std::shared_ptr& context, T&& a) { context->_holds[index] = - std::move(a).recover([_context = std::weak_ptr(context), _i = index](auto x) { + std::move(a).recover(std::forward(executor), [_context = std::weak_ptr(context), _i = index](auto x) { auto p = _context.lock(); if (!p) return; auto error = x.error(); @@ -1429,11 +1423,9 @@ struct create_range_of_futures> { size_t index(0); for (; first != last; ++first) { - attach_tasks(index++, context, *first); + attach_tasks(index++, executor, context, *first); } - executor(std::move(p.first)); - return std::move(p.second); } }; @@ -1452,11 +1444,9 @@ struct create_range_of_futures> { size_t index(0); for (; first != last; ++first) { - attach_tasks(index++, context, std::forward(*first)); + attach_tasks(index++, executor, context, std::forward(*first)); } - executor(std::move(p.first)); - return std::move(p.second); } }; @@ -1677,11 +1667,11 @@ void shared_base::set_value(F& f, Args&&... args) { /**************************************************************************************************/ -template -auto shared_base::recover(S s, F&& f) +template +auto shared_base::recover(E executor, F&& f) -> future)>>> { auto p = package)>()>( - s, [_f = std::forward(f), _p = future(this->shared_from_this())]() mutable { + executor, [_f = std::forward(f), _p = future(this->shared_from_this())]() mutable { return _f(_p); }); @@ -1689,9 +1679,9 @@ auto shared_base::recover(S s, F&& f) { std::unique_lock lock(_mutex); ready = _ready; - if (!ready) _then.emplace_back(std::move(s), std::move(p.first)); + if (!ready) _then.emplace_back(std::move(executor), std::move(p.first)); } - if (ready) s(std::move(p.first)); + if (ready) executor(std::move(p.first)); return reduce(std::move(p.second)); } diff --git a/test/future_test_helper.hpp b/test/future_test_helper.hpp index dc4b466ad..b03571e71 100644 --- a/test/future_test_helper.hpp +++ b/test/future_test_helper.hpp @@ -25,8 +25,7 @@ template struct custom_scheduler { using result_type = void; - template - void operator()(F f) { + void operator()(stlab::task f) const { ++counter(); // The implementation on Windows or the mac uses a scheduler that allows many tasks in the // pool in parallel @@ -49,7 +48,7 @@ struct custom_scheduler { } private: - const size_t _id = no; // only used for debugging purpose + size_t _id = no; // only used for debugging purpose }; class test_exception : public std::exception { diff --git a/test/future_when_all_arguments_tests.cpp b/test/future_when_all_arguments_tests.cpp index 0001b2257..4747f85af 100644 --- a/test/future_when_all_arguments_tests.cpp +++ b/test/future_when_all_arguments_tests.cpp @@ -54,6 +54,46 @@ BOOST_AUTO_TEST_CASE(future_when_all_args_int_with_many_elements) { BOOST_REQUIRE_LE(4, custom_scheduler<0>::usage_counter()); BOOST_REQUIRE_LE(1, custom_scheduler<1>::usage_counter()); } + +BOOST_AUTO_TEST_CASE(future_when_all_args_int_with_ready_element) { + BOOST_TEST_MESSAGE("running future when_all int with ready element"); + + sut = when_all(custom_scheduler<1>(), [](auto x) { return x + x; }, make_ready_future(42, immediate_executor)); + + check_valid_future(sut); + wait_until_future_completed(sut); + + BOOST_REQUIRE_EQUAL(42 + 42, *sut.get_try()); + BOOST_REQUIRE_LE(1, custom_scheduler<1>::usage_counter()); +} + +BOOST_AUTO_TEST_CASE(future_when_all_args_int_with_two_ready_element) { + BOOST_TEST_MESSAGE("running future when_all int with two ready element"); + + sut = when_all(custom_scheduler<1>(), [](auto x, auto y) { return x + y; }, + make_ready_future(42, immediate_executor), + make_ready_future(42, immediate_executor)); + + check_valid_future(sut); + wait_until_future_completed(sut); + + BOOST_REQUIRE_EQUAL(42 + 42, *sut.get_try()); + BOOST_REQUIRE_LE(1, custom_scheduler<1>::usage_counter()); +} + +BOOST_AUTO_TEST_CASE(future_when_all_args) { + + auto main_thread_id = std::this_thread::get_id(); + auto sut = when_all(custom_scheduler<1>(), [] { + return std::this_thread::get_id(); + }, make_ready_future(stlab::immediate_executor)); + + wait_until_future_completed(sut); + + BOOST_REQUIRE(main_thread_id != *sut.get_try()); + BOOST_REQUIRE_LE(1, custom_scheduler<1>::usage_counter()); +} + BOOST_AUTO_TEST_SUITE_END() BOOST_FIXTURE_TEST_SUITE(future_when_all_args_string, test_fixture) diff --git a/test/future_when_any_range_tests.cpp b/test/future_when_any_range_tests.cpp index c6fbabdd0..2682c7b9e 100644 --- a/test/future_when_any_range_tests.cpp +++ b/test/future_when_any_range_tests.cpp @@ -605,7 +605,7 @@ BOOST_AUTO_TEST_CASE(future_when_any_move_only_range_with_diamond_formation_elem sut = when_any(custom_scheduler<1>(), [&_i = index](stlab::move_only x, size_t index) { _i = index; - return std::move(x); + return x; }, std::make_pair(futures.begin(), futures.end())); From 8a37fc9a452daadd4d6262d31cec514ef16f9196 Mon Sep 17 00:00:00 2001 From: Felix Petriconi Date: Sat, 28 Jul 2018 15:38:48 +0200 Subject: [PATCH 3/4] Renaming the remaining template parameter S to E --- stlab/concurrency/future.hpp | 92 ++++++++++++++++++------------------ 1 file changed, 46 insertions(+), 46 deletions(-) diff --git a/stlab/concurrency/future.hpp b/stlab/concurrency/future.hpp index c8c35c685..9829d7719 100644 --- a/stlab/concurrency/future.hpp +++ b/stlab/concurrency/future.hpp @@ -432,9 +432,9 @@ struct shared_base> : std::enable_shared_from_this< return then_r(unique, _executor, std::forward(f)); } - template - auto then_r(bool unique, S&& s, F&& f) { - return recover_r(unique, std::forward(s), [_f = std::forward(f)](auto&& x) mutable { + template + auto then_r(bool unique, E&& executor, F&& f) { + return recover_r(unique, std::forward(executor), [_f = std::forward(f)](auto&& x) mutable { return std::move(_f)(std::move(*std::forward(x).get_try())); }); } @@ -524,9 +524,9 @@ struct shared_base : std::enable_shared_from_this> { return then(_executor, std::forward(f)); } - template - auto then(S&& s, F&& f) { - return recover(std::forward(s), [_f = std::forward(f)](auto x) mutable { + template + auto then(E&& executor, F&& f) { + return recover(std::forward(executor), [_f = std::forward(f)](auto x) mutable { x.get_try(); // throw if error return std::move(_f)(); }); @@ -537,9 +537,9 @@ struct shared_base : std::enable_shared_from_this> { return then(_executor, std::forward(f)); } - template - auto then_r(bool, S&& s, F&& f) { - return then(std::forward(s), std::forward(f)); + template + auto then_r(bool, E&& executor, F&& f) { + return then(std::forward(executor), std::forward(f)); } template @@ -547,17 +547,17 @@ struct shared_base : std::enable_shared_from_this> { return recover(_executor, std::forward(f)); } - template - auto recover(S s, F&& f) -> future)>>>; + template + auto recover(E&& executor, F&& f) -> future)>>>; template auto recover_r(bool, F&& f) { return recover(_executor, std::forward(f)); } - template - auto recover_r(bool, S&& s, F&& f) { - return recover(std::forward(s), std::forward(f)); + template + auto recover_r(bool, E&& executor, F&& f) { + return recover(std::forward(executor), std::forward(f)); } template @@ -746,9 +746,9 @@ class future> { return _p->then(std::forward(f)); } - template - auto then(S&& s, F&& f) const& { - return _p->then(std::forward(s), std::forward(f)); + template + auto then(E&& executor, F&& f) const& { + return _p->then(std::forward(executor), std::forward(f)); } template @@ -756,9 +756,9 @@ class future> { return _p->then_r(unique_usage(_p), std::forward(f)); } - template - auto then(S&& s, F&& f) && { - return _p->then_r(unique_usage(_p), std::forward(s), std::forward(f)); + template + auto then(E&& executor, F&& f) && { + return _p->then_r(unique_usage(_p), std::forward(executor), std::forward(f)); } template @@ -766,9 +766,9 @@ class future> { return _p->recover(std::forward(f)); } - template - auto recover(S&& s, F&& f) const& { - return _p->recover(std::forward(s), std::forward(f)); + template + auto recover(E&& executor, F&& f) const& { + return _p->recover(std::forward(executor), std::forward(f)); } template @@ -776,9 +776,9 @@ class future> { return _p->recover_r(unique_usage(_p), std::forward(f)); } - template - auto recover(S&& s, F&& f) && { - return _p->recover_r(unique_usage(_p), std::forward(s), std::forward(f)); + template + auto recover(E&& executor, F&& f) && { + return _p->recover_r(unique_usage(_p), std::forward(executor), std::forward(f)); } void detach() const { @@ -837,9 +837,9 @@ class future { return _p->then(std::forward(f)); } - template - auto then(S&& s, F&& f) const& { - return _p->then(std::forward(s), std::forward(f)); + template + auto then(E&& executor, F&& f) const& { + return _p->then(std::forward(executor), std::forward(f)); } template @@ -847,9 +847,9 @@ class future { return _p->then_r(unique_usage(_p), std::forward(f)); } - template - auto then(S&& s, F&& f) && { - return _p->then_r(unique_usage(_p), std::forward(s), std::forward(f)); + template + auto then(E&& executor, F&& f) && { + return _p->then_r(unique_usage(_p), std::forward(executor), std::forward(f)); } template @@ -857,9 +857,9 @@ class future { return _p->recover(std::forward(f)); } - template - auto recover(S&& s, F&& f) const& { - return _p->recover(std::forward(s), std::forward(f)); + template + auto recover(E&& executor, F&& f) const& { + return _p->recover(std::forward(executor), std::forward(f)); } template @@ -867,9 +867,9 @@ class future { return _p->recover_r(unique_usage(_p), std::forward(f)); } - template - auto recover(S&& s, F&& f) && { - return _p->recover_r(unique_usage(_p), std::forward(s), std::forward(f)); + template + auto recover(E&& executor, F&& f) && { + return _p->recover_r(unique_usage(_p), std::forward(executor), std::forward(f)); } void detach() const { @@ -930,9 +930,9 @@ class future> { return _p->then_r(unique_usage(_p), std::forward(f)); } - template - auto then(S&& s, F&& f) && { - return _p->then_r(unique_usage(_p), std::forward(s), std::forward(f)); + template + auto then(E&& executor, F&& f) && { + return _p->then_r(unique_usage(_p), std::forward(executor), std::forward(f)); } template @@ -940,9 +940,9 @@ class future> { return _p->recover_r(unique_usage(_p), std::forward(f)); } - template - auto recover(S&& s, F&& f) && { - return _p->recover_r(unique_usage(_p), std::forward(s), std::forward(f)); + template + auto recover(E&& executor, F&& f) && { + return _p->recover_r(unique_usage(_p), std::forward(executor), std::forward(f)); } void detach() const { @@ -1668,7 +1668,7 @@ void shared_base::set_value(F& f, Args&&... args) { /**************************************************************************************************/ template -auto shared_base::recover(E executor, F&& f) +auto shared_base::recover(E&& executor, F&& f) -> future)>>> { auto p = package)>()>( executor, [_f = std::forward(f), _p = future(this->shared_from_this())]() mutable { @@ -1679,7 +1679,7 @@ auto shared_base::recover(E executor, F&& f) { std::unique_lock lock(_mutex); ready = _ready; - if (!ready) _then.emplace_back(std::move(executor), std::move(p.first)); + if (!ready) _then.emplace_back(std::forward(executor), std::move(p.first)); } if (ready) executor(std::move(p.first)); From bc2a7f8ecd595b0eec9fbde4cc6846c48ec1a408 Mon Sep 17 00:00:00 2001 From: Felix Petriconi Date: Sat, 28 Jul 2018 15:48:40 +0200 Subject: [PATCH 4/4] Update version information --- CHANGES.md | 5 +++++ stlab/version.hpp | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index ec7812dd3..f2ac931da 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,8 @@ +## v1.3.2 - 2018 - July - 28 + +- Fixed Issues + - [#169](https://github.com/stlab/libraries/issues/169) : when_all() appears to ignore the passed-in executor + ## v1.3.1 - 2018 - July - 21 - Fixed Issues diff --git a/stlab/version.hpp b/stlab/version.hpp index 1709f51a2..568ba7234 100644 --- a/stlab/version.hpp +++ b/stlab/version.hpp @@ -19,13 +19,13 @@ // STLAB_VERSION / 100 % 1000 is the minor version // STLAB_VERSION / 100000 is the major version -#define STLAB_VERSION 100301 +#define STLAB_VERSION 100302 // // STLAB_LIB_VERSION must be defined to be the same as STLAB_VERSION // but as a *string* in the form "x_y[_z]" where x is the major version // number, y is the minor version number, and z is the patch level if not 0. -#define STLAB_LIB_VERSION "1_3_1" +#define STLAB_LIB_VERSION "1_3_2" #endif