Disallow redundant Bind calls.

Disallow redundant Bind calls.

Make it a compile-time error base::Bind a base::Callback as the functor
object with no additional args: in that case, just use the original
callback!

BUG=none
R=danakj@chromium.org,tzik@chromium.org
TBR=brettw@chromium.org

Committed: https://crrev.com/172b6ad8f0213ba74d9447dbd08e32e1c7ce655a
Cr-Commit-Position: refs/heads/master@{#420826}

[dcheng, 2016-09-19 19:26:05]

Description was changed from

==========
Disallow pointless Bind calls.

Make it a compile-time error base::Bind a base::Callback as the functor
object with no additional args: in that case, just use the original
callback!

BUG=none
==========

to

==========
Disallow pointless Bind calls.

Make it a compile-time error base::Bind a base::Callback as the functor
object with no additional args: in that case, just use the original
callback!

BUG=none
==========

[dcheng, 2016-09-19 19:26:05]

dcheng@chromium.org changed reviewers:
+ danakj@chromium.org

[dcheng, 2016-09-19 19:37:12]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 19:37:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2016-09-19 19:37:53]

Inspired by https://codereview.chromium.org/2299523003/. I'm not sure if our
nocompile tests still work now that GYP is gone...

[dcheng, 2016-09-19 19:44:51]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 19:45:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-09-19 19:48:58]

LGTM

[commit-bot: I haz the power, 2016-09-19 19:55:53]

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

[commit-bot: I haz the power, 2016-09-19 19:55:54]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[dcheng, 2016-09-19 23:06:35]

Description was changed from

==========
Disallow pointless Bind calls.

Make it a compile-time error base::Bind a base::Callback as the functor
object with no additional args: in that case, just use the original
callback!

BUG=none
==========

to

==========
Disallow redundant Bind calls.

Make it a compile-time error base::Bind a base::Callback as the functor
object with no additional args: in that case, just use the original
callback!

BUG=none
==========

[dcheng, 2016-09-19 23:59:13]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 00:00:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 00:19:45]

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

[commit-bot: I haz the power, 2016-09-20 00:19:46]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[dcheng, 2016-09-20 00:39:58]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 00:40:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 01:35:33]

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

[commit-bot: I haz the power, 2016-09-20 01:35:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tzik, 2016-09-20 05:12:09]

tzik@chromium.org changed reviewers:
+ tzik@chromium.org

[tzik, 2016-09-20 05:12:10]

lgtm

[tzik, 2016-09-20 07:33:05]

https://codereview.chromium.org/2352853002/diff/120001/base/bind_internal.h
File base/bind_internal.h (right):

https://codereview.chromium.org/2352853002/diff/120001/base/bind_internal.h#n...
base/bind_internal.h:434: struct BindingCallbackWithNoArgs<Callback<Signature>,
BoundArgs...> {
Could you add a specialization for OnceCallback too?

[dcheng, 2016-09-20 08:22:39]

https://codereview.chromium.org/2352853002/diff/120001/base/bind_internal.h
File base/bind_internal.h (right):

https://codereview.chromium.org/2352853002/diff/120001/base/bind_internal.h#n...
base/bind_internal.h:434: struct BindingCallbackWithNoArgs<Callback<Signature>,
BoundArgs...> {
On 2016/09/20 07:33:04, tzik wrote:
> Could you add a specialization for OnceCallback too?

How come things like CancellationChecker don't currently specialize on
OnceCallback?

[tzik, 2016-09-20 09:51:28]

On 2016/09/20 08:22:39, dcheng wrote:
> https://codereview.chromium.org/2352853002/diff/120001/base/bind_internal.h
> File base/bind_internal.h (right):
> 
>
https://codereview.chromium.org/2352853002/diff/120001/base/bind_internal.h#n...
> base/bind_internal.h:434: struct
BindingCallbackWithNoArgs<Callback<Signature>,
> BoundArgs...> {
> On 2016/09/20 07:33:04, tzik wrote:
> > Could you add a specialization for OnceCallback too?
> 
> How come things like CancellationChecker don't currently specialize on
> OnceCallback?

It's a bug also on CancellationChecker. And I'm fixing it:
https://codereview.chromium.org/2355763002/

[dcheng, 2016-09-24 00:49:17]

PTAL. A few rebases got dragged in, but the importants diffs between PS7 and PS8
are in //base.

https://codereview.chromium.org/2352853002/diff/120001/base/bind_internal.h
File base/bind_internal.h (right):

https://codereview.chromium.org/2352853002/diff/120001/base/bind_internal.h#n...
base/bind_internal.h:434: struct BindingCallbackWithNoArgs<Callback<Signature>,
BoundArgs...> {
On 2016/09/20 07:33:04, tzik wrote:
> Could you add a specialization for OnceCallback too?

Done (though I did this by templatizing on the additional parameters)

https://codereview.chromium.org/2352853002/diff/140001/base/bind_unittest.nc
File base/bind_unittest.nc (right):

https://codereview.chromium.org/2352853002/diff/140001/base/bind_unittest.nc#...
base/bind_unittest.nc:215: internal::OnceClosure cb2 = internal::BindOnce(cb);
This technically fails for two reasons: cb is not copyable (we should use
std::move) and the static_assert.

Since the nocompile driver doesn't know where to find the libc++ headers, I
could add a rvalue static cast here if anyone feels strongly about tying it to
only one error.

This has the nice property of verifying that you get two errors for the price of
one though? So maybe we can just keep this way?

[dcheng, 2016-09-24 00:52:56]

https://codereview.chromium.org/2352853002/diff/140001/base/bind_unittest.nc
File base/bind_unittest.nc (right):

https://codereview.chromium.org/2352853002/diff/140001/base/bind_unittest.nc#...
base/bind_unittest.nc:215: internal::OnceClosure cb2 = internal::BindOnce(cb);
On 2016/09/24 00:49:17, dcheng wrote:
> This technically fails for two reasons: cb is not copyable (we should use
> std::move) and the static_assert.
> 
> Since the nocompile driver doesn't know where to find the libc++ headers, I
> could add a rvalue static cast here if anyone feels strongly about tying it to
> only one error.
> 
> This has the nice property of verifying that you get two errors for the price
of
> one though? So maybe we can just keep this way?

Actually, I had a typo the first time. Updated this to just use std::move() and
verify the nocompile test still passes (e.g. compilation fails)

[danakj, 2016-09-24 01:20:01]

LGTM

https://codereview.chromium.org/2352853002/diff/160001/device/serial/data_sen...
File device/serial/data_sender.cc (left):

https://codereview.chromium.org/2352853002/diff/160001/device/serial/data_sen...
device/serial/data_sender.cc:127: pending_cancel_.Reset();
can you still reset after in case code checks if its null, or is move from
well-defined to leave it null for this?

[dcheng, 2016-09-24 01:29:57]

https://codereview.chromium.org/2352853002/diff/160001/device/serial/data_sen...
File device/serial/data_sender.cc (left):

https://codereview.chromium.org/2352853002/diff/160001/device/serial/data_sen...
device/serial/data_sender.cc:127: pending_cancel_.Reset();
On 2016/09/24 01:20:01, danakj wrote:
> can you still reset after in case code checks if its null, or is move from
> well-defined to leave it null for this?

I'm going to add a unit test for this... but this reminds me...

This doesn't actually work for whatever reason. I'm going to revert this to the
naive version (just without the base::Bind) and debug / land some unit tests in
a followup.

[dcheng, 2016-09-24 01:51:32]

Description was changed from

==========
Disallow redundant Bind calls.

Make it a compile-time error base::Bind a base::Callback as the functor
object with no additional args: in that case, just use the original
callback!

BUG=none
==========

to

==========
Disallow redundant Bind calls.

Make it a compile-time error base::Bind a base::Callback as the functor
object with no additional args: in that case, just use the original
callback!

BUG=none
R=danakj@chromium.org,tzik@chromium.org
==========

[dcheng, 2016-09-24 01:51:33]

dcheng@chromium.org changed reviewers:
+ brettw@chromium.org

[dcheng, 2016-09-24 01:51:56]

Description was changed from

==========
Disallow redundant Bind calls.

Make it a compile-time error base::Bind a base::Callback as the functor
object with no additional args: in that case, just use the original
callback!

BUG=none
R=danakj@chromium.org,tzik@chromium.org
==========

to

==========
Disallow redundant Bind calls.

Make it a compile-time error base::Bind a base::Callback as the functor
object with no additional args: in that case, just use the original
callback!

BUG=none
R=danakj@chromium.org,tzik@chromium.org
TBR=brettw@chromium.org
==========

[dcheng, 2016-09-24 02:00:33]

TBRing brettw for global approval (danakj@ and tzik@ have already reviewed the
entire CL. Outside of //base, it's just deletions of base::Bind() or adding an
additional template specialization for callback forwarding templates with no
arguments)

[dcheng, 2016-09-24 02:00:44]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-09-24 02:00:45]

The patchset sent to the CQ was uploaded after l-g-t-m from tzik@chromium.org,
danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2352853002/#ps200001
(title: "static_assert message tweak")

[commit-bot: I haz the power, 2016-09-24 02:01:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-24 04:05:12]

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

[commit-bot: I haz the power, 2016-09-24 04:05:13]

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

[dcheng, 2016-09-24 04:12:07]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-09-24 04:12:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-24 05:06:28]

Description was changed from

==========
Disallow redundant Bind calls.

Make it a compile-time error base::Bind a base::Callback as the functor
object with no additional args: in that case, just use the original
callback!

BUG=none
R=danakj@chromium.org,tzik@chromium.org
TBR=brettw@chromium.org
==========

to

==========
Disallow redundant Bind calls.

Make it a compile-time error base::Bind a base::Callback as the functor
object with no additional args: in that case, just use the original
callback!

BUG=none
R=danakj@chromium.org,tzik@chromium.org
TBR=brettw@chromium.org
==========

[commit-bot: I haz the power, 2016-09-24 05:06:29]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-09-24 05:08:19]

Description was changed from

==========
Disallow redundant Bind calls.

Make it a compile-time error base::Bind a base::Callback as the functor
object with no additional args: in that case, just use the original
callback!

BUG=none
R=danakj@chromium.org,tzik@chromium.org
TBR=brettw@chromium.org
==========

to

==========
Disallow redundant Bind calls.

Make it a compile-time error base::Bind a base::Callback as the functor
object with no additional args: in that case, just use the original
callback!

BUG=none
R=danakj@chromium.org,tzik@chromium.org
TBR=brettw@chromium.org

Committed: https://crrev.com/172b6ad8f0213ba74d9447dbd08e32e1c7ce655a
Cr-Commit-Position: refs/heads/master@{#420826}
==========

[commit-bot: I haz the power, 2016-09-24 05:08:21]

Patchset 11 (id:??) landed as
https://crrev.com/172b6ad8f0213ba74d9447dbd08e32e1c7ce655a
Cr-Commit-Position: refs/heads/master@{#420826}