Remove `Runnable` concept from base/bind_internal.h

Remove `Runnable` concept from base/bind_internal.h

`Runnable` concept was useful when bind_internal.h was a huge generated
file. However, it's an unneeded layer these days.
This CL removes `Runnable` from bind_internal.h to simplify its impl.
After this CL, BindState holds the bound Functor directly in its internal
storage.

This CL contains:
 * Merge bind_internal_win.h into bind_internal.h
 * Move typechecks in Bind() into MakeBindStateType.
 * Remove no longer used HasIsMethodTag in bind_helper.h.
 * Remove InvokeHelper specialization for IgnoreResult.
 * Merge assertion-only InvokeHelper specialization into another
   specialization.
 * Factor out the type setup in BindState into MakeBindStateType.

BUG=554299
Committed: https://crrev.com/99de02ba952b0a69291f81c5b8ca14d81cc1f74f
Cr-Commit-Position: refs/heads/master@{#403413}

[tzik, 2016-06-28 14:46:50]

Description was changed from

==========
Remove `Runnable` concept from base/bind_internal.h

BUG=
==========

to

==========
Remove `Runnable` concept from base/bind_internal.h

123456789012345678901234567890123456789012345678901234567890123456789012

TODO: description

BUG=
==========

[tzik, 2016-06-29 12:25:52]

Description was changed from

==========
Remove `Runnable` concept from base/bind_internal.h

123456789012345678901234567890123456789012345678901234567890123456789012

TODO: description

BUG=
==========

to

==========
Remove `Runnable` concept from base/bind_internal.h

This CL contains:
 * Merge bind_internal_win.h into bind_internal.h
 * Move typechecks in Bind() into MakeBindStateType.
 * Remove unused bind_helper.h stuff.
 * Remove `Runnable` concept from bind_internal.h, and move most of its
   functionality to FunctorTrait.
 * Remove InvokeHelper specialization for IgnoreResult.
 * Merge assertion-only InvokeHelper specialization into another
   specialization.
 * Factor out the type setup in BindState into MakeBindStateType.

BUG=
==========

[tzik, 2016-06-29 13:03:14]

Description was changed from

==========
Remove `Runnable` concept from base/bind_internal.h

This CL contains:
 * Merge bind_internal_win.h into bind_internal.h
 * Move typechecks in Bind() into MakeBindStateType.
 * Remove unused bind_helper.h stuff.
 * Remove `Runnable` concept from bind_internal.h, and move most of its
   functionality to FunctorTrait.
 * Remove InvokeHelper specialization for IgnoreResult.
 * Merge assertion-only InvokeHelper specialization into another
   specialization.
 * Factor out the type setup in BindState into MakeBindStateType.

BUG=
==========

to

==========
Remove `Runnable` concept from base/bind_internal.h

This CL contains:
 * Merge bind_internal_win.h into bind_internal.h
 * Move typechecks in Bind() into MakeBindStateType.
 * Remove no longer used HasIsMethodTag in bind_helper.h.
 * Remove `Runnable` concept from bind_internal.h, and move most of its
   functionality to FunctorTrait.
 * Remove InvokeHelper specialization for IgnoreResult.
 * Merge assertion-only InvokeHelper specialization into another
   specialization.
 * Factor out the type setup in BindState into MakeBindStateType.

BUG=
==========

[tzik, 2016-06-29 13:06:03]

Description was changed from

==========
Remove `Runnable` concept from base/bind_internal.h

This CL contains:
 * Merge bind_internal_win.h into bind_internal.h
 * Move typechecks in Bind() into MakeBindStateType.
 * Remove no longer used HasIsMethodTag in bind_helper.h.
 * Remove `Runnable` concept from bind_internal.h, and move most of its
   functionality to FunctorTrait.
 * Remove InvokeHelper specialization for IgnoreResult.
 * Merge assertion-only InvokeHelper specialization into another
   specialization.
 * Factor out the type setup in BindState into MakeBindStateType.

BUG=
==========

to

==========
Remove `Runnable` concept from base/bind_internal.h

This CL contains:
 * Merge bind_internal_win.h into bind_internal.h
 * Move typechecks in Bind() into MakeBindStateType.
 * Remove no longer used HasIsMethodTag in bind_helper.h.
 * Remove `Runnable` concept from bind_internal.h, and move most of its
   functionality to FunctorTrait.
 * Remove InvokeHelper specialization for IgnoreResult.
 * Merge assertion-only InvokeHelper specialization into another
   specialization.
 * Factor out the type setup in BindState into MakeBindStateType.

BUG=554299
==========

[tzik, 2016-06-29 13:19:36]

Description was changed from

==========
Remove `Runnable` concept from base/bind_internal.h

This CL contains:
 * Merge bind_internal_win.h into bind_internal.h
 * Move typechecks in Bind() into MakeBindStateType.
 * Remove no longer used HasIsMethodTag in bind_helper.h.
 * Remove `Runnable` concept from bind_internal.h, and move most of its
   functionality to FunctorTrait.
 * Remove InvokeHelper specialization for IgnoreResult.
 * Merge assertion-only InvokeHelper specialization into another
   specialization.
 * Factor out the type setup in BindState into MakeBindStateType.

BUG=554299
==========

to

==========
Remove `Runnable` concept from base/bind_internal.h

`Runnable` concept was useful when bind_internal.h was a huge generated
file. However, it's an unneeded layer these days.
This CL removes `Runnable` from bind_internal.h to simplify its impl.
After this CL, BindState holds the bound Functor directly in its internal
storage.

This CL contains:
 * Merge bind_internal_win.h into bind_internal.h
 * Move typechecks in Bind() into MakeBindStateType.
 * Remove no longer used HasIsMethodTag in bind_helper.h.
 * Remove InvokeHelper specialization for IgnoreResult.
 * Merge assertion-only InvokeHelper specialization into another
   specialization.
 * Factor out the type setup in BindState into MakeBindStateType.

BUG=554299
==========

[tzik, 2016-06-29 13:23:02]

tzik@chromium.org changed reviewers:
+ dcheng@chromium.org, yutak@chromium.org

[tzik, 2016-06-29 13:23:03]

PTAL

[tzik, 2016-06-29 16:08:01]

On 2016/06/29 13:23:03, tzik wrote:
> PTAL

win-clang failure seems a clang-cl bug.
I filed it as https://llvm.org/bugs/show_bug.cgi?id=28360

[Yuta Kitamura, 2016-06-30 07:13:29]

Looked throughly and seemed a nice refactoring. LGTM, but I'd
expect another review from someone who's more knowledgable in
this code.

https://codereview.chromium.org/2106773002/diff/140001/base/bind_internal.h
File base/bind_internal.h (right):

https://codereview.chromium.org/2106773002/diff/140001/base/bind_internal.h#n...
base/bind_internal.h:32: //             a Run() function.  Usually just a
convenience typedef.
Apparently Run() in this context is no more?

https://codereview.chromium.org/2106773002/diff/140001/base/bind_internal.h#n...
base/bind_internal.h:247: BoundArgsTuple&& bound,
Both |functor| and |bound| are always lvalue in this case, aren't they?
Especially, std::forward() calls below seem just passing an lvalue
reference; if that's the case, std::forward() call is an excessive
decoration.

(This has existed before your change, so it's okay to ignore.)

[dcheng, 2016-06-30 12:53:37]

Looks good overall. Just one small question and one nit

https://codereview.chromium.org/2106773002/diff/140001/base/bind_internal.h
File base/bind_internal.h (right):

https://codereview.chromium.org/2106773002/diff/140001/base/bind_internal.h#n...
base/bind_internal.h:131: RunArgs&&... args) {
Is the clang CV qualifier check comment in the old RunnableAdapter impl
obsolete?

https://codereview.chromium.org/2106773002/diff/140001/base/bind_internal.h#n...
base/bind_internal.h:278: 
Nit: I think the other top-level definitions in this file are separated by only
one newline, so let's remove this for consistency?

[tzik, 2016-07-01 04:11:00]

https://codereview.chromium.org/2106773002/diff/140001/base/bind_internal.h
File base/bind_internal.h (right):

https://codereview.chromium.org/2106773002/diff/140001/base/bind_internal.h#n...
base/bind_internal.h:32: //             a Run() function.  Usually just a
convenience typedef.
On 2016/06/30 07:13:29, Yuta Kitamura wrote:
> Apparently Run() in this context is no more?

Changed this to Callback<>::Run().

https://codereview.chromium.org/2106773002/diff/140001/base/bind_internal.h#n...
base/bind_internal.h:131: RunArgs&&... args) {
On 2016/06/30 12:53:36, dcheng wrote:
> Is the clang CV qualifier check comment in the old RunnableAdapter impl
> obsolete?

Oh, I missed to move it. It's still valid, and NCTEST_METHOD_ON_CONST_OBJECT in
bind_unittest.nc catches it.
Updated the comment.

https://codereview.chromium.org/2106773002/diff/140001/base/bind_internal.h#n...
base/bind_internal.h:247: BoundArgsTuple&& bound,
On 2016/06/30 07:13:29, Yuta Kitamura wrote:
> Both |functor| and |bound| are always lvalue in this case, aren't they?
> Especially, std::forward() calls below seem just passing an lvalue
> reference; if that's the case, std::forward() call is an excessive
> decoration.
> 
> (This has existed before your change, so it's okay to ignore.)

Good point. But let me leave this as is for future use.
I'm going to add another variant of Invoker::Run in next CL,
Invoker::RunOneShot, that will pass rvalue references to RunImpl.

https://codereview.chromium.org/2106773002/diff/140001/base/bind_internal.h#n...
base/bind_internal.h:278: 
On 2016/06/30 12:53:36, dcheng wrote:
> Nit: I think the other top-level definitions in this file are separated by
only
> one newline, so let's remove this for consistency?

Done.

[dcheng, 2016-07-01 04:20:40]

lgtm

[tzik, 2016-07-01 05:22:55]

The CQ bit was checked by tzik@chromium.org

[tzik, 2016-07-01 05:22:56]

The patchset sent to the CQ was uploaded after l-g-t-m from yutak@chromium.org
Link to the patchset: https://codereview.chromium.org/2106773002/#ps160001
(title: "+comment. fix spacing.")

[commit-bot: I haz the power, 2016-07-01 05:23:03]

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-07-01 05:54:27]

Description was changed from

==========
Remove `Runnable` concept from base/bind_internal.h

`Runnable` concept was useful when bind_internal.h was a huge generated
file. However, it's an unneeded layer these days.
This CL removes `Runnable` from bind_internal.h to simplify its impl.
After this CL, BindState holds the bound Functor directly in its internal
storage.

This CL contains:
 * Merge bind_internal_win.h into bind_internal.h
 * Move typechecks in Bind() into MakeBindStateType.
 * Remove no longer used HasIsMethodTag in bind_helper.h.
 * Remove InvokeHelper specialization for IgnoreResult.
 * Merge assertion-only InvokeHelper specialization into another
   specialization.
 * Factor out the type setup in BindState into MakeBindStateType.

BUG=554299
==========

to

==========
Remove `Runnable` concept from base/bind_internal.h

`Runnable` concept was useful when bind_internal.h was a huge generated
file. However, it's an unneeded layer these days.
This CL removes `Runnable` from bind_internal.h to simplify its impl.
After this CL, BindState holds the bound Functor directly in its internal
storage.

This CL contains:
 * Merge bind_internal_win.h into bind_internal.h
 * Move typechecks in Bind() into MakeBindStateType.
 * Remove no longer used HasIsMethodTag in bind_helper.h.
 * Remove InvokeHelper specialization for IgnoreResult.
 * Merge assertion-only InvokeHelper specialization into another
   specialization.
 * Factor out the type setup in BindState into MakeBindStateType.

BUG=554299
==========

[commit-bot: I haz the power, 2016-07-01 05:54:28]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-07-01 05:56:43]

Description was changed from

==========
Remove `Runnable` concept from base/bind_internal.h

`Runnable` concept was useful when bind_internal.h was a huge generated
file. However, it's an unneeded layer these days.
This CL removes `Runnable` from bind_internal.h to simplify its impl.
After this CL, BindState holds the bound Functor directly in its internal
storage.

This CL contains:
 * Merge bind_internal_win.h into bind_internal.h
 * Move typechecks in Bind() into MakeBindStateType.
 * Remove no longer used HasIsMethodTag in bind_helper.h.
 * Remove InvokeHelper specialization for IgnoreResult.
 * Merge assertion-only InvokeHelper specialization into another
   specialization.
 * Factor out the type setup in BindState into MakeBindStateType.

BUG=554299
==========

to

==========
Remove `Runnable` concept from base/bind_internal.h

`Runnable` concept was useful when bind_internal.h was a huge generated
file. However, it's an unneeded layer these days.
This CL removes `Runnable` from bind_internal.h to simplify its impl.
After this CL, BindState holds the bound Functor directly in its internal
storage.

This CL contains:
 * Merge bind_internal_win.h into bind_internal.h
 * Move typechecks in Bind() into MakeBindStateType.
 * Remove no longer used HasIsMethodTag in bind_helper.h.
 * Remove InvokeHelper specialization for IgnoreResult.
 * Merge assertion-only InvokeHelper specialization into another
   specialization.
 * Factor out the type setup in BindState into MakeBindStateType.

BUG=554299

Committed: https://crrev.com/99de02ba952b0a69291f81c5b8ca14d81cc1f74f
Cr-Commit-Position: refs/heads/master@{#403413}
==========

[commit-bot: I haz the power, 2016-07-01 05:56:44]

Patchset 9 (id:??) landed as
https://crrev.com/99de02ba952b0a69291f81c5b8ca14d81cc1f74f
Cr-Commit-Position: refs/heads/master@{#403413}

[pdknsk, 2016-12-09 23:20:56]

This has likely introduced a bug, or revealed a compiler bug, with non-default
build options.

https://bugs.chromium.org/p/chromium/issues/detail?id=671140#c8