Allow PostTask() to take unique_ptr to classes derived of QueuedTask

Allow PostTask() to take unique_ptr to classes derived of QueuedTask

Problem fixed by this CL: Let DerivedQueuedTask be a custom derivation of QueuedTask. Calling PostTask() with a std::unique_ptr<DerivedQueuedTask> does not work, because overload resolution sees PostTask(const Closure& closure) as a better match. The workaround of explicitly converting to std::unique_ptr<QueuedTask> before calling PostTask() results in less readable code.

Solution: Use std::enable_if to limit the template, thereby making the compiler use the right version of PostTask().

BUG=webrtc:8188
Review-Url: https://codereview.webrtc.org/3006933002
Cr-Commit-Position: refs/heads/master@{#19625}
Committed: https://chromium.googlesource.com/external/webrtc/+/ffe2e141834d80e12a41251a5dfe4ad9581a1baa

[eladalon, 2017-08-31 08:59:52]

eladalon@webrtc.org changed reviewers:
+ kwiberg@webrtc.org, nisse@webrtc.org

[eladalon, 2017-08-31 08:59:53]

PTAL

[nisse-webrtc, 2017-08-31 09:28:53]

lgtm

Description nit: Replace "yields in" by "yields" or "results in".

[nisse-webrtc, 2017-08-31 09:40:39]

https://codereview.webrtc.org/3006933002/diff/1/webrtc/rtc_base/task_queue.h
File webrtc/rtc_base/task_queue.h (right):

https://codereview.webrtc.org/3006933002/diff/1/webrtc/rtc_base/task_queue.h#...
webrtc/rtc_base/task_queue.h:199: void PostTask(const Closure& closure) {
One question: At some point, we'll move to C++17, and then we can have lambdas
doing capture-by-move, including by capturing unique_ptr. And we'd then want
Closure to be able to match such lambdas, which I imagine will *not* be
copy-constructible. I don't understand all the magic, but to me, it seems that
when we get there, we'd need to

1. Adjust the template, and maybe the ClosureTask class too, to accept a Closure
which isn't copy-constructible.

2. Add a more narrow exclusion, to exclude std::unique_ptr<T> it and only if
that's implicitly convertible to std::unique_ptr<QueuedTask>.

Is it possible to do any of that right away? Or will it be possible with C++17?

[kwiberg-webrtc, 2017-08-31 10:45:33]

https://codereview.webrtc.org/3006933002/diff/1/webrtc/rtc_base/task_queue.h
File webrtc/rtc_base/task_queue.h (right):

https://codereview.webrtc.org/3006933002/diff/1/webrtc/rtc_base/task_queue.h#...
webrtc/rtc_base/task_queue.h:25: #include "webrtc/rtc_base/type_traits.h"
You should #include <type_traits> instead.

https://codereview.webrtc.org/3006933002/diff/1/webrtc/rtc_base/task_queue.h#...
webrtc/rtc_base/task_queue.h:199: void PostTask(const Closure& closure) {
On 2017/08/31 09:40:39, nisse-webrtc wrote:
> One question: At some point, we'll move to C++17, and then we can
> have lambdas doing capture-by-move, including by capturing
> unique_ptr.

Yes. (But it's C++14, not 17.)

> And we'd then want Closure to be able to match such lambdas,

Yes.

> which I imagine will *not* be
> copy-constructible.

That's right.

(It's already possible to construct such objects in C++11---you just
can't do it with lambdas alone. You need to manually write a class, or
wrap a lambda in bind guck.)

> I don't understand all the magic, but to me, it seems that when we
> get there, we'd need to
> 
> 1. Adjust the template, and maybe the ClosureTask class too, to
> accept a Closure which isn't copy-constructible.

Yes. We'd want to std::move it everywhere.

> 2. Add a more narrow exclusion, to exclude std::unique_ptr<T> it and
> only if that's implicitly convertible to
> std::unique_ptr<QueuedTask>.
> 
> Is it possible to do any of that right away? Or will it be possible
> with C++17?

I don't know offhand how to test if a type is unique_ptr<T> for some
T. However, we do know that closure objects need to be callable, and
we also know that unique_ptrs aren't callable. C++17 has a type trait
called std::is_invocable that tests if a type can be called with a
given argument list, and we can implement something similar in C++11
if we wanted to---see rtc::HasDataAndSize for an example.

I don't have a strong opinion on whether it's best to just use
std::is_copy_constructible for now, or if we should define and use
rtc::IsCallable. The latter is prettier, because it expresses what we
really mean, but it's also more work since we have to write
rtc::IsCallable.

[eladalon, 2017-08-31 10:46:47]

Description was changed from

==========
Allow PostTask() to take unique_ptr to classes derived of QueuedTask

Problem fixed by this CL: Let DerivedQueuedTask be a custom derivation of
QueuedTask. Calling PostTask() with a std::unique_ptr<DerivedQueuedTask> does
not work, because overload resolution sees PostTask(const Closure& closure) as a
better match. The workaround of explicitly converting to
std::unique_ptr<QueuedTask> before calling PostTask() yields in less readable
code.

Solution: Use std::enable_if to limit the template, thereby making the compiler
use the right version of PostTask().

BUG=webrtc:8188
==========

to

==========
Allow PostTask() to take unique_ptr to classes derived of QueuedTask

Problem fixed by this CL: Let DerivedQueuedTask be a custom derivation of
QueuedTask. Calling PostTask() with a std::unique_ptr<DerivedQueuedTask> does
not work, because overload resolution sees PostTask(const Closure& closure) as a
better match. The workaround of explicitly converting to
std::unique_ptr<QueuedTask> before calling PostTask() results in less readable
code.

Solution: Use std::enable_if to limit the template, thereby making the compiler
use the right version of PostTask().

BUG=webrtc:8188
==========

[eladalon, 2017-08-31 10:51:04]

https://codereview.webrtc.org/3006933002/diff/1/webrtc/rtc_base/task_queue.h
File webrtc/rtc_base/task_queue.h (right):

https://codereview.webrtc.org/3006933002/diff/1/webrtc/rtc_base/task_queue.h#...
webrtc/rtc_base/task_queue.h:25: #include "webrtc/rtc_base/type_traits.h"
On 2017/08/31 10:45:33, kwiberg-webrtc wrote:
> You should #include <type_traits> instead.

Done.

[kwiberg-webrtc, 2017-08-31 10:53:02]

lgtm

Your call if you want to put in the work required to enable the template based
on whether the closure type is callable rather than copy constructible.

[nisse-webrtc, 2017-08-31 11:00:07]

On 2017/08/31 10:45:33, kwiberg-webrtc wrote:
> I don't know offhand how to test if a type is unique_ptr<T> for some
> T. However, we do know that closure objects need to be callable, and
> we also know that unique_ptrs aren't callable. C++17 has a type trait
> called std::is_invocable that tests if a type can be called with a
> given argument list, and we can implement something similar in C++11
> if we wanted to---see rtc::HasDataAndSize for an example.

Since (1) callable types and relevant unique_ptr types are disjoint, and (2)
callability really is what the ClosureTask needs, that sounds like the right way
to handle it. But not necessarily in this cl.

[eladalon, 2017-08-31 11:02:09]

The CQ bit was checked by eladalon@webrtc.org

[eladalon, 2017-08-31 11:02:10]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org
Link to the patchset: https://codereview.webrtc.org/3006933002/#ps20001 (title:
".")

[commit-bot: I haz the power, 2017-08-31 11:02:15]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-31 11:36:05]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1504177329678910,
"parent_rev": "6e09d875fb28e49029fac798382e2c8df4a1f752", "commit_rev":
"ffe2e141834d80e12a41251a5dfe4ad9581a1baa"}

[commit-bot: I haz the power, 2017-08-31 11:36:12]

Description was changed from

==========
Allow PostTask() to take unique_ptr to classes derived of QueuedTask

Problem fixed by this CL: Let DerivedQueuedTask be a custom derivation of
QueuedTask. Calling PostTask() with a std::unique_ptr<DerivedQueuedTask> does
not work, because overload resolution sees PostTask(const Closure& closure) as a
better match. The workaround of explicitly converting to
std::unique_ptr<QueuedTask> before calling PostTask() results in less readable
code.

Solution: Use std::enable_if to limit the template, thereby making the compiler
use the right version of PostTask().

BUG=webrtc:8188
==========

to

==========
Allow PostTask() to take unique_ptr to classes derived of QueuedTask

Problem fixed by this CL: Let DerivedQueuedTask be a custom derivation of
QueuedTask. Calling PostTask() with a std::unique_ptr<DerivedQueuedTask> does
not work, because overload resolution sees PostTask(const Closure& closure) as a
better match. The workaround of explicitly converting to
std::unique_ptr<QueuedTask> before calling PostTask() results in less readable
code.

Solution: Use std::enable_if to limit the template, thereby making the compiler
use the right version of PostTask().

BUG=webrtc:8188

Review-Url: https://codereview.webrtc.org/3006933002
Cr-Commit-Position: refs/heads/master@{#19625}
Committed:
https://chromium.googlesource.com/external/webrtc/+/ffe2e141834d80e12a41251a5...
==========

[commit-bot: I haz the power, 2017-08-31 11:36:13]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/ffe2e141834d80e12a41251a5...