[Base] Use variadic template in base/callback.h (wave 1)

[Base] Use variadic template in base/callback.h

Replace pump.py generated base/callback.h with variadic template version.

BUG=433164
Committed: https://crrev.com/c87149e666acfe39d49d7f30a5fd9acc7ef085ea
Cr-Commit-Position: refs/heads/master@{#304948}

[tzik, 2014-09-29 18:36:37]

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

[tzik, 2014-09-29 18:44:02]

Hi, Albert. PTL.

C++11 variadic template seems ready to use in chrome.
I tried to make a CL to use it in Callback and Bind, though it doesn't compile
on Windows yet.
https://codereview.chromium.org/610313003/
Can we start using it from an easy piece?

[awong, 2014-09-29 18:51:48]

LGTM

Hell yes. Thank you for doing this.

[awong, 2014-09-29 18:52:28]

On 2014/09/29 18:51:48, awong (On leave) wrote:
> LGTM
> 
> Hell yes. Thank you for doing this.

Please also watch the build stats. I think we're going to see some speed ups
(esp when we replace bind_internal) and it'd be nice to publish those results.

[tzik, 2014-09-30 04:49:36]

On 2014/09/29 18:52:28, awong (On leave) wrote:
> On 2014/09/29 18:51:48, awong (On leave) wrote:
> > LGTM
> > 
> > Hell yes. Thank you for doing this.
> 
> Please also watch the build stats. I think we're going to see some speed ups
> (esp when we replace bind_internal) and it'd be nice to publish those results.

Looks like we hit a compiler bug, reported as below.
http://connect.microsoft.com/VisualStudio/feedbackdetail/view/957801/compilat...

They said it is fixed in 'the "14" release of Visual C++', which is the next
major release of MSVC.

[tzik, 2014-09-30 09:30:42]

PTAL.

I added a workaround to CallbackParamTraits for MSVC 2013.
It seems we can't use default template parameter and variadic template together.

[Nico, 2014-09-30 11:41:03]

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

[Nico, 2014-09-30 11:41:04]

https://codereview.chromium.org/610423003/diff/20001/base/callback_internal.h
File base/callback_internal.h (right):

https://codereview.chromium.org/610423003/diff/20001/base/callback_internal.h...
base/callback_internal.h:99: struct CallbackParamTraitsForNonMoveOnlyType;
Should there be a "TODO: Use a default parameter once MSVS supports variadic
templates with default values"?

[Nico, 2014-09-30 12:00:44]

(independent of your answer to my question, I like this cl.)

On Tue, Sep 30, 2014 at 1:41 PM, <thakis@chromium.org> wrote:

>
> https://codereview.chromium.org/610423003/diff/20001/base/
> callback_internal.h
> File base/callback_internal.h (right):
>
> https://codereview.chromium.org/610423003/diff/20001/base/
> callback_internal.h#newcode99
> base/callback_internal.h:99: struct
> CallbackParamTraitsForNonMoveOnlyType;
> Should there be a "TODO: Use a default parameter once MSVS supports
> variadic templates with default values"?
>
> https://codereview.chromium.org/610423003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tzik, 2014-09-30 14:43:10]

https://codereview.chromium.org/610423003/diff/20001/base/callback_internal.h
File base/callback_internal.h (right):

https://codereview.chromium.org/610423003/diff/20001/base/callback_internal.h...
base/callback_internal.h:99: struct CallbackParamTraitsForNonMoveOnlyType;
On 2014/09/30 11:41:04, Nico (away until Oct 27) wrote:
> Should there be a "TODO: Use a default parameter once MSVS supports variadic
> templates with default values"?

Done.

[tzik, 2014-09-30 15:00:00]

Surprisingly, monolithic version of .pump replacement CL compiles on Windows
without additional workaround!
https://codereview.chromium.org/610313003/#ps120001

[Aaron Boodman, 2014-09-30 21:05:36]

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

[Aaron Boodman, 2014-09-30 21:05:37]

https://codereview.chromium.org/610423003/diff/20001/base/callback_internal.h
File base/callback_internal.h (right):

https://codereview.chromium.org/610423003/diff/20001/base/callback_internal.h...
base/callback_internal.h:84: // Returns Then as |type| if condition is true.
Otherwise returns Else.
The pipes are usually used around param names in chromium, so i think:

Returns |Then| as SelectType::type when |condition| is true. Otherwise returns
|Else|.

[tzik, 2014-10-01 01:22:41]

https://codereview.chromium.org/610423003/diff/20001/base/callback_internal.h
File base/callback_internal.h (right):

https://codereview.chromium.org/610423003/diff/20001/base/callback_internal.h...
base/callback_internal.h:84: // Returns Then as |type| if condition is true.
Otherwise returns Else.
On 2014/09/30 21:05:37, Aaron Boodman wrote:
> The pipes are usually used around param names in chromium, so i think:
> 
> Returns |Then| as SelectType::type when |condition| is true. Otherwise returns
> |Else|.

Done.

[Aaron Boodman, 2014-10-03 23:26:16]

lgtm

[awong, 2014-10-07 18:45:39]

https://codereview.chromium.org/610423003/diff/80001/base/callback.h
File base/callback.h (right):

https://codereview.chromium.org/610423003/diff/80001/base/callback.h#newcode411
base/callback.h:411: #endif  // BASE_CALLBACK_H_
Oooh...nice catch.

https://codereview.chromium.org/610423003/diff/80001/base/callback_internal.h
File base/callback_internal.h (right):

https://codereview.chromium.org/610423003/diff/80001/base/callback_internal.h...
base/callback_internal.h:103: // with default values.
Can we cite a bit that has a reference to the MSVS issue that's requiring this
code change?

[tzik, 2014-10-08 15:56:11]

https://codereview.chromium.org/610423003/diff/80001/base/callback_internal.h
File base/callback_internal.h (right):

https://codereview.chromium.org/610423003/diff/80001/base/callback_internal.h...
base/callback_internal.h:103: // with default values.
On 2014/10/07 18:45:39, awong (On leave) wrote:
> Can we cite a bit that has a reference to the MSVS issue that's requiring this
> code change?

Added an upstream bug link.
Though, actually, I still don't understand when the compiler fails to compile,
and which usage we should really avoid...

I removed SFINAE/default template parameters, variable number of inheritance in
single level, complex template specialization match, and multiple template pack
expansion in single expression from the following CL.
Each of them works fine individually though.

[tzik, 2014-10-14 06:47:54]

Albert: Could you take another look to this?
I made a non-trivial change for MSVC2013 workaround after your LGTM in #3.

[Aaron Boodman, 2014-10-31 18:56:33]

aa@chromium.org changed reviewers:
- aa@chromium.org

[tzik, 2014-11-17 07:38:29]

Now the Windows toolchain is updated to MSVS Community 2013!
That should compile this correctly.
I rebased CL onto the ToT, the diff itself is not changed since last upload.

Albert: Could you PTAL to the workaround part between PS1 and PS2?

[Nico, 2014-11-20 00:00:18]

diff from patch set 1 to 2 lgtm (albert seems away)

Let's get this in, before another 2 people implement this :-/

[Nico, 2014-11-20 00:04:19]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2014-11-20 00:06:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/610423003/120001

[commit-bot: I haz the power, 2014-11-20 01:08:30]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-11-20 01:09:14]

Patchset 7 (id:??) landed as
https://crrev.com/c87149e666acfe39d49d7f30a5fd9acc7ef085ea
Cr-Commit-Position: refs/heads/master@{#304948}