base: Stop using Pass() on move-only types in Bind and Callback.

base: Stop using Pass() on move-only types in Bind and Callback.

This replaces calls to .Pass() with calls to std::move() instead, which
will help allow us to remove the .Pass() method off of our move-only
types.

It does not dis/allow any new behaviour with Bind and Callback.
Specifically:

1. PassedWrapper is created from base::Passed. Previously PassedWrapper
called .Pass() which required a whitelisted type to compile. Now we get
a much nicer error by using enable_if on base::Passed to check that the
type is whitelisted:

../../base/callback_unittest.cc:221:66: error: no matching function for call to 'Passed'
 Callback<void(void)> callback = base::Bind(&TakeUniqueByValue, base::Passed(&i));
                                                                ^~~~~~~~~~~~

This prevents use such as base::Passed(&a_std_unique_ptr).

2. The CallbackParamTraits specialize on the whitelist, and for non-
whitelisted types it will store a T and pass a const T&. The receiving
method will take a T which will try to copy and fail for non-whitelisted
move-only types. Secondly, the constructor of BindState will fail as
Bind takes a const T& for each bound argument currently, and the storage
of T will try to copy into the BindState.

This prevents use such as base::Bind(&F, std::unique_ptr<T>()), which will
also apply to scoped_ptr.

R=Nico, dcheng
BUG=557422
Committed: https://crrev.com/314d1f44144e9d2a12d41445c9e5a1287e14373c
Cr-Commit-Position: refs/heads/master@{#363635}

[danakj, 2015-12-04 20:46:13]

https://codereview.chromium.org/1496403002/diff/1/base/bind_helpers.h
File base/bind_helpers.h (right):

https://codereview.chromium.org/1496403002/diff/1/base/bind_helpers.h#newcode375
base/bind_helpers.h:375: explicit PassedWrapper(T&& scoper)
This take a T&&, but the T is a class template type, which means this is
strictly a T&&, not a universal reference.

This use of T&& prevents one move constructor from being called on Bind().

https://codereview.chromium.org/1496403002/diff/1/base/bind_helpers.h#newcode586
base/bind_helpers.h:586: static inline internal::PassedWrapper<T> Passed(T&&
scoper) {
This one doesn't prevent any move constructors (I guess receiving by value into
a local and moving it in an inline method and all it compiles away). But I
thought it represents semantically what this is receiving better. WDYT?

[danakj, 2015-12-04 20:50:17]

https://codereview.chromium.org/1496403002/diff/1/base/bind_helpers.h
File base/bind_helpers.h (right):

https://codereview.chromium.org/1496403002/diff/1/base/bind_helpers.h#newcode586
base/bind_helpers.h:586: static inline internal::PassedWrapper<T> Passed(T&&
scoper) {
On 2015/12/04 20:46:13, danakj (behind on reviews) wrote:
> This one doesn't prevent any move constructors (I guess receiving by value
into
> a local and moving it in an inline method and all it compiles away). But I
> thought it represents semantically what this is receiving better. WDYT?

I thought about this some more and I don't think I would ask other people to
write && in cases like this.. I'll remove this one.

[dcheng, 2015-12-04 20:59:08]

https://codereview.chromium.org/1496403002/diff/1/base/bind_helpers.h
File base/bind_helpers.h (right):

https://codereview.chromium.org/1496403002/diff/1/base/bind_helpers.h#newcode586
base/bind_helpers.h:586: static inline internal::PassedWrapper<T> Passed(T&&
scoper) {
On 2015/12/04 at 20:50:16, danakj (behind on reviews) wrote:
> On 2015/12/04 20:46:13, danakj (behind on reviews) wrote:
> > This one doesn't prevent any move constructors (I guess receiving by value
into
> > a local and moving it in an inline method and all it compiles away). But I
> > thought it represents semantically what this is receiving better. WDYT?
> 
> I thought about this some more and I don't think I would ask other people to
write && in cases like this.. I'll remove this one.

I think what you're seeing is copy elision. Is the compiler smart enough to
elide this if it's called as:
  base::Passed(std::move(x))
?

[danakj, 2015-12-04 21:05:26]

https://codereview.chromium.org/1496403002/diff/1/base/bind_helpers.h
File base/bind_helpers.h (right):

https://codereview.chromium.org/1496403002/diff/1/base/bind_helpers.h#newcode586
base/bind_helpers.h:586: static inline internal::PassedWrapper<T> Passed(T&&
scoper) {
On 2015/12/04 20:59:08, dcheng wrote:
> On 2015/12/04 at 20:50:16, danakj (behind on reviews) wrote:
> > On 2015/12/04 20:46:13, danakj (behind on reviews) wrote:
> > > This one doesn't prevent any move constructors (I guess receiving by value
> into
> > > a local and moving it in an inline method and all it compiles away). But I
> > > thought it represents semantically what this is receiving better. WDYT?
> > 
> > I thought about this some more and I don't think I would ask other people to
> write && in cases like this.. I'll remove this one.
> 
> I think what you're seeing is copy elision. Is the compiler smart enough to
> elide this if it's called as:
>   base::Passed(std::move(x))
> ?

Oh, nope. It saves a move constructor to use T&& here in that case. Thanks.

[danakj, 2015-12-04 21:07:06]

PTAL

[dcheng, 2015-12-04 22:07:08]

lgtm

https://codereview.chromium.org/1496403002/diff/60001/base/bind_helpers.h
File base/bind_helpers.h (right):

https://codereview.chromium.org/1496403002/diff/60001/base/bind_helpers.h#new...
base/bind_helpers.h:580: // via use of enable_if, and the second takes a T*
which will not bind to a T
Nit. I think you can just say T* which will not bind to T&.

[Nico, 2015-12-04 22:08:16]

lgtm

https://codereview.chromium.org/1496403002/diff/60001/base/bind_helpers.h
File base/bind_helpers.h (right):

https://codereview.chromium.org/1496403002/diff/60001/base/bind_helpers.h#new...
base/bind_helpers.h:575: // is best suited for use with the return value of a
function of other temporary
s/of other/or other/

[danakj, 2015-12-04 22:09:30]

https://codereview.chromium.org/1496403002/diff/60001/base/bind_helpers.h
File base/bind_helpers.h (right):

https://codereview.chromium.org/1496403002/diff/60001/base/bind_helpers.h#new...
base/bind_helpers.h:575: // is best suited for use with the return value of a
function of other temporary
On 2015/12/04 22:08:16, Nico wrote:
> s/of other/or other/

Done.

https://codereview.chromium.org/1496403002/diff/60001/base/bind_helpers.h#new...
base/bind_helpers.h:580: // via use of enable_if, and the second takes a T*
which will not bind to a T
On 2015/12/04 22:07:08, dcheng wrote:
> Nit. I think you can just say T* which will not bind to T&.

Done.

[danakj, 2015-12-04 22:09:50]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-12-04 22:09:50]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1496403002/#ps80001
(title: "bind-pass: fixes")

[commit-bot: I haz the power, 2015-12-04 22:12:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1496403002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1496403002/80001

[commit-bot: I haz the power, 2015-12-04 23:01:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-04 23:01:09]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[danakj, 2015-12-04 23:01:53]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-12-04 23:20:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1496403002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1496403002/80001

[commit-bot: I haz the power, 2015-12-05 00:24:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-05 00:24:17]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)

[danakj, 2015-12-05 00:27:36]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-12-05 00:29:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1496403002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1496403002/80001

[commit-bot: I haz the power, 2015-12-05 02:42:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-05 02:42:02]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)

[danakj, 2015-12-07 18:56:50]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-12-07 18:59:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1496403002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1496403002/80001

[commit-bot: I haz the power, 2015-12-07 22:22:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-07 22:22:56]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[danakj, 2015-12-07 22:23:11]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-12-07 22:29:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1496403002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1496403002/80001

[commit-bot: I haz the power, 2015-12-08 00:44:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-08 00:47:01]

Description was changed from

==========
base: Stop using Pass() on move-only types in Bind and Callback.

This replaces calls to .Pass() with calls to std::move() instead, which
will help allow us to remove the .Pass() method off of our move-only
types.

It does not dis/allow any new behaviour with Bind and Callback.
Specifically:

1. PassedWrapper is created from base::Passed. Previously PassedWrapper
called .Pass() which required a whitelisted type to compile. Now we get
a much nicer error by using enable_if on base::Passed to check that the
type is whitelisted:

../../base/callback_unittest.cc:221:66: error: no matching function for call to
'Passed'
 Callback<void(void)> callback = base::Bind(&TakeUniqueByValue,
base::Passed(&i));
                                                                ^~~~~~~~~~~~

This prevents use such as base::Passed(&a_std_unique_ptr).

2. The CallbackParamTraits specialize on the whitelist, and for non-
whitelisted types it will store a T and pass a const T&. The receiving
method will take a T which will try to copy and fail for non-whitelisted
move-only types. Secondly, the constructor of BindState will fail as
Bind takes a const T& for each bound argument currently, and the storage
of T will try to copy into the BindState.

This prevents use such as base::Bind(&F, std::unique_ptr<T>()), which will
also apply to scoped_ptr.

R=Nico, dcheng
BUG=557422
==========

to

==========
base: Stop using Pass() on move-only types in Bind and Callback.

This replaces calls to .Pass() with calls to std::move() instead, which
will help allow us to remove the .Pass() method off of our move-only
types.

It does not dis/allow any new behaviour with Bind and Callback.
Specifically:

1. PassedWrapper is created from base::Passed. Previously PassedWrapper
called .Pass() which required a whitelisted type to compile. Now we get
a much nicer error by using enable_if on base::Passed to check that the
type is whitelisted:

../../base/callback_unittest.cc:221:66: error: no matching function for call to
'Passed'
 Callback<void(void)> callback = base::Bind(&TakeUniqueByValue,
base::Passed(&i));
                                                                ^~~~~~~~~~~~

This prevents use such as base::Passed(&a_std_unique_ptr).

2. The CallbackParamTraits specialize on the whitelist, and for non-
whitelisted types it will store a T and pass a const T&. The receiving
method will take a T which will try to copy and fail for non-whitelisted
move-only types. Secondly, the constructor of BindState will fail as
Bind takes a const T& for each bound argument currently, and the storage
of T will try to copy into the BindState.

This prevents use such as base::Bind(&F, std::unique_ptr<T>()), which will
also apply to scoped_ptr.

R=Nico, dcheng
BUG=557422

Committed: https://crrev.com/314d1f44144e9d2a12d41445c9e5a1287e14373c
Cr-Commit-Position: refs/heads/master@{#363635}
==========

[commit-bot: I haz the power, 2015-12-08 00:47:02]

Patchset 5 (id:??) landed as
https://crrev.com/314d1f44144e9d2a12d41445c9e5a1287e14373c
Cr-Commit-Position: refs/heads/master@{#363635}