PPLX move-only task parameter support #1231

miherius · 2019-08-25T23:50:06Z

It is possible now to pass move-only objects as task parameter to the pplx::task constructor and to the pplx::create_task() function. So, code below is compilable now:

auto moveOnlyPtr = std::unique_ptr<Data>();
auto task = pplx::create_task( [data = std::move(moveOnlyPtr)]() {/*process data*/} );

Also, optimization comes as a free bonus, since we don't copy task parameter. For example, code below:

pplx::create_task( [v = std::move(hugeVector)]() {/*process v*/} ).get();

in current master realization it performs 7 copies and 2 moves of v object;
in this pull request realization it makes no copies, and performs 2 moves of v object.

Another minor interface refactor - redundant pplx::task constructor has been removed for better match with pplx::create_task() overloads.

This pull request is similar to #47 change. And like in #47 change, all pplx-specific tests were placed under corresponding #ifdef.

Checked in GCC 7.4.0 and MSVC2017 (both native windows ppl implementation, and forced pplx).

(functor copy count reduced from 7 to 5)

…ut compiler errors. Also static_assert for task result type check added.

…pass void-result tce only into void-result task. Also adding tests for these cases.

…etween task<T> and task<void> classes.

…) function by introducing forwarding-reference parameter.

… chain.

…-support

…or from details::_FilterValidTaskType() function, so initial tasks can be constructed from move-only objects.

…-support

`TestTasks_move_only_then` test logic moved to `create_task_with_move_only_continuation` test.

msftclas · 2019-08-25T23:50:20Z

All CLA requirements met.

BillyONeal

Hmmm I'm not sure how reasonable this is; we generally want pplx to have the exact same API as PPL, and PPL can never take such a change due to ABI restrictions (it often puts the callables into a std::function).

BillyONeal · 2019-08-27T01:01:47Z

Release/include/pplx/pplxtasks.h

@@ -3066,25 +3081,58 @@ auto _IsValidTaskCtor(_Ty _Param, int, int, ...)
    -> decltype(_Param.set(stdx::declval<_ReturnType>()), std::true_type());

 template<typename _ReturnType, typename _Ty>
-auto _IsValidTaskCtor(_Ty _Param, int, ...) -> decltype(_Param.set(), std::true_type());
+auto _IsValidTaskCtor(_Ty _Param, int, ...)
+    -> decltype(_Param.set(), typename std::enable_if<std::is_same<_ReturnType, void>::value>::type(), std::true_type());


This change appears to rely on expression SFINAE which is not safe to use on one of our supported compilers, VS2015.

I believe that existing logic like decltype(_Param.set(), /*...*/), which check a method existence, already use expression SFINAE. So I decided that this change should be safe. Anyway, I just tried to compile it in VS2015 (update 3) with forced PPLX, and it seems to work there.
Furthermore, VS2015 is not using PPLX by default, and this code enabled only for PPLX.

BillyONeal · 2019-08-27T01:04:02Z

Release/include/pplx/pplxtasks.h

+template<typename _Ty, typename _ReturnType, typename =
+    typename std::enable_if<!std::is_base_of<task<_ReturnType>, _Ty>::value>::type
+>
+struct _EnableIfNotTaskRet


You probably want to derive from enable_if rather than making formation of _EnableIfNotTaskRet itself impossible.

I agree, this is not a common practice. Changed to inheritance for _EnableIfNotTaskRet and _EnableIfNotTask structs in 19f0b83 and 0df92d9

BillyONeal · 2019-08-27T01:05:32Z

Release/include/pplx/pplxtasks.h

@@ -4391,11 +4408,11 @@ class task<void>
    ///     Runtime)"/>.</para>
    /// </remarks>
    /**/
-    template<typename _Ty>
+    template<typename _Ty, typename = typename details::_EnableIfNotTaskRet<typename std::decay<_Ty>::type, void>>


Conventionally, preventing this from being a special member function should use the injected class name; e.g. `typename = enable_if_t<is_same_v<task, decay_t<_Ty>>>"

(Occurs elsewhere)

This is exactly what _EnableIfNotTaskRet doing: checks that _Ty is not derived from task<_ReturnType> (is_base_of check includes is_same logic in this case). So we can catch that tricky case when _Ty is not a task class itself, but its child class, when compiler may decide to use this forwarding-reference overload instead of copy- or move-constructor.

BillyONeal · 2019-08-27T01:06:46Z

Release/tests/functional/pplx/pplx_test/pplxtask_tests.cpp

+    }
+#endif // !(defined(_MSC_VER) && (_MSC_VER >= 1800) && !CPPREST_FORCE_PPLX)
+
+//TODO: tests for _AsyncTaskCtorArgsValidator (__cplusplus_winrt only)


We shouldn't check in TODO comments; feel free to file an 'XXX should be tested' issue.

I noticed that __cplusplus_winrt stuff is not presented in tests at all, so I removed this TODO (6d8c603)

BillyONeal · 2019-08-27T01:12:14Z

Release/tests/functional/pplx/pplx_test/pplxtask_tests.cpp

+        VERIFY_ARE_EQUAL(t2.get(), 22);
+    }
+
+#if !(defined(_MSC_VER) && (_MSC_VER >= 1800) && !CPPREST_FORCE_PPLX)


If _MSC_VER is not defined, then it can't be defined to greater than 1800. Perhaps demorgan?

#if !defined(_MSC_VER) || CPPREST_FORCE_PPLX

?

But when _MSC_VER is defined, it can be less than 1800, and this should make this #if work.

P.S. This all is just opposite condition from the beginning of the pplxtasks.h file, where choose between PPL and pplx is made.

…nable_if`.

miherius · 2019-09-01T23:48:36Z

we generally want pplx to have the exact same API as PPL

I tried to save existing compatibility conditions. And I used interface deviations brought by #47 as a reference. That is, when you can use #47's stuff, you can use this pull-request's stuff, and vice-versa. The only difference in place where movable-only functor can appear: continuation in case of #47 and initial task in case of current pull-request.

…le_if`. Plus, fixing GCC compiling error related to `_EnableIfNotTaskRet` using.

…-support

…lusplus_winrt`.

miherius · 2019-09-02T06:55:50Z

All failed tests throws exception Unhandled exception: bind: Address already in use. And each of them tried to listen localhost:45678 address. So I think it should be a test environment issue.

BillyONeal · 2020-02-19T00:23:31Z

I'm sorry but as I indicated, for API compatibility reasons between PPL and PPLX I don't think I'm able to merge this :(

miherius added 17 commits July 11, 2019 21:16

Don't make runtime calls just to make compile-time checks

3e7be8b

(functor copy count reduced from 7 to 5)

Adding tests for task ctor static validator, so we can check it witho…

a7ebc88

…ut compiler errors. Also static_assert for task result type check added.

Fixing task creation from task_completion_event validation: allow to …

1f00462

…pass void-result tce only into void-result task. Also adding tests for these cases.

Merge branch 'master' into pplx-move-only-task-support

4db5f7a

Remove redundant task ctor - this give more interface compatibility b…

f855e10

…etween task<T> and task<void> classes.

Removing of redundant parameter copying by task ctor and create_task(…

de8f503

…) function by introducing forwarding-reference parameter.

Remove any copy operations (like std::function) for initial task call…

1359162

… chain.

Merge remote-tracking branch 'origin/master' into pplx-move-only-task…

5ae4fe3

…-support

Removing implicit access to the copy-ctor of the initial task's funct…

9a39f81

…or from details::_FilterValidTaskType() function, so initial tasks can be constructed from move-only objects.

Fixing of copy-paste error in TestTasks_move_only_initial_task

abe8fed

Merge remote-tracking branch 'remotes/microsoft/master'

42ca19d

Merge remote-tracking branch 'remotes/microsoft/master'

9a6512b

Merge remote-tracking branch 'origin/master' into pplx-move-only-task…

bb4722d

…-support

Tests for pplx tasks move semantics.

8709510

`TestTasks_move_only_then` test logic moved to `create_task_with_move_only_continuation` test.

Code formatting: tabs to whitespaces.

acc6213

Move all pplx-related tests under one #ifdef.

6e842d0

Fix few typos.

0beca7e

BillyONeal requested changes Aug 27, 2019

View reviewed changes

miherius added 2 commits September 1, 2019 14:55

Merge remote-tracking branch 'remotes/microsoft/master'

73a306e

Making details::_EnableIfNotTaskRet struct behave more like `std::e…

19f0b83

…nable_if`.

miherius added 3 commits September 2, 2019 02:53

Making details::_EnableIfNotTask struct behave more like `std::enab…

0df92d9

…le_if`. Plus, fixing GCC compiling error related to `_EnableIfNotTaskRet` using.

Merge remote-tracking branch 'origin/master' into pplx-move-only-task…

c566e29

…-support

Remove TODO comment, as current branch changes are unrelated to `__cp…

6d8c603

…lusplus_winrt`.

BillyONeal closed this Feb 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

PPLX move-only task parameter support #1231

PPLX move-only task parameter support #1231

Uh oh!

miherius commented Aug 25, 2019

Uh oh!

msftclas commented Aug 25, 2019 •

edited

Loading

Uh oh!

BillyONeal left a comment

Uh oh!

BillyONeal Aug 27, 2019

Uh oh!

miherius Sep 1, 2019

Uh oh!

BillyONeal Aug 27, 2019

Uh oh!

miherius Sep 2, 2019

Uh oh!

BillyONeal Aug 27, 2019

Uh oh!

miherius Sep 1, 2019

Uh oh!

BillyONeal Aug 27, 2019

Uh oh!

miherius Sep 2, 2019

Uh oh!

BillyONeal Aug 27, 2019

Uh oh!

miherius Sep 2, 2019

Uh oh!

miherius commented Sep 1, 2019

Uh oh!

miherius commented Sep 2, 2019

Uh oh!

BillyONeal commented Feb 19, 2020

Uh oh!

Uh oh!

PPLX move-only task parameter support #1231

PPLX move-only task parameter support #1231

Uh oh!

Conversation

miherius commented Aug 25, 2019

Uh oh!

msftclas commented Aug 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BillyONeal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miherius commented Sep 1, 2019

Uh oh!

miherius commented Sep 2, 2019

Uh oh!

BillyONeal commented Feb 19, 2020

Uh oh!

Uh oh!

msftclas commented Aug 25, 2019 •

edited

Loading