Add new helper that can adapt Callbacks with return values for Closures.

Add new helper that can adapt Callbacks with return values for Closures.

This is helpful for reducing manually written static functions that just effectively just invoke the function and ignore the return value.

BUG=87287
TEST=new unittest

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=103060

[willchan no longer on Chromium, 2011-09-27 03:30:45]

LGTM but wait for Darin too.

[darin (slow to review), 2011-09-27 16:48:00]

http://codereview.chromium.org/8048008/diff/1/base/bind_helpers.h
File base/bind_helpers.h (right):

http://codereview.chromium.org/8048008/diff/1/base/bind_helpers.h#newcode56
base/bind_helpers.h:56: //   int DoSomething(int arg) { cout << arg << end; }
nit: "end" -> "endl"

http://codereview.chromium.org/8048008/diff/1/base/bind_helpers.h#newcode57
base/bind_helpers.h:57: //   Callback <int(void)> cb = Bind(&DoSomething, 1);
nit: no space before "<"

http://codereview.chromium.org/8048008/diff/1/base/bind_helpers.h#newcode58
base/bind_helpers.h:58: //   Closure c = IgnoreReturn(cb);  // Prints "1"
All of the use cases I can imagine would basically look like:

  IgnoreReturn(Bind(...))

Would it make more sense to just have:

  BindIgnoringReturn(...)

Or, would that result in too much template bloat?

[awong, 2011-09-27 16:56:05]

On Tue, Sep 27, 2011 at 9:48 AM, <darin@chromium.org> wrote:

>
>
http://codereview.chromium.**org/8048008/diff/1/base/bind_**helpers.h<http://...
> File base/bind_helpers.h (right):
>
> http://codereview.chromium.**org/8048008/diff/1/base/bind_**
>
helpers.h#newcode56<http://codereview.chromium.org/8048008/diff/1/base/bind_helpers.h#newcode56>
> base/bind_helpers.h:56: //   int DoSomething(int arg) { cout << arg <<
> end; }
> nit: "end" -> "endl"
>
> http://codereview.chromium.**org/8048008/diff/1/base/bind_**
>
helpers.h#newcode57<http://codereview.chromium.org/8048008/diff/1/base/bind_helpers.h#newcode57>
> base/bind_helpers.h:57: //   Callback <int(void)> cb =
> Bind(&DoSomething, 1);
> nit: no space before "<"
>
> http://codereview.chromium.**org/8048008/diff/1/base/bind_**
>
helpers.h#newcode58<http://codereview.chromium.org/8048008/diff/1/base/bind_helpers.h#newcode58>
> base/bind_helpers.h:58: //   Closure c = IgnoreReturn(cb);  // Prints
> "1"
> All of the use cases I can imagine would basically look like:
>
>  IgnoreReturn(Bind(...))
>
> Would it make more sense to just have:
>
>  BindIgnoringReturn(...)
>
> Or, would that result in too much template bloat?


(heading in...will answer other points later).

I like the wrapping version a bit better since it has less bloat, but also
because I think there's a reasonable case in the future to just remove
IgnoreReturn, and modify the type system of Callback to specifically allow
removing the return value.

So, for any set of parameter A..., base::Callback<void(A...)> should just be
assignable from base::Callback<R(A...)>.

This is type safe because void(A...) is a supertype of R(A...).  Really,
T(A...) is a supertype of R(A...) as long as T is a supertype of R.
 Tr1::bind allows for these assignments, and adding support in for us isn't
super hard.  I had just avoided it initially because I wanted to make sure
we had a need for it first.

-Albert

[willchan no longer on Chromium, 2011-09-27 17:02:23]

Roughly agreed with Albert although I'd push back on the implicit conversion
harder. I'm in general biased towards explicit rather than implicit
conversions. But if this happens too many times (where "too many" is subject
to opinion), I'll cave in.

On Tue, Sep 27, 2011 at 9:55 AM, Albert J. Wong (王重傑)
<ajwong@chromium.org>wrote:

>
>
> On Tue, Sep 27, 2011 at 9:48 AM, <darin@chromium.org> wrote:
>
>>
>>
http://codereview.chromium.**org/8048008/diff/1/base/bind_**helpers.h<http://...
>> File base/bind_helpers.h (right):
>>
>> http://codereview.chromium.**org/8048008/diff/1/base/bind_**
>>
helpers.h#newcode56<http://codereview.chromium.org/8048008/diff/1/base/bind_helpers.h#newcode56>
>> base/bind_helpers.h:56: //   int DoSomething(int arg) { cout << arg <<
>> end; }
>> nit: "end" -> "endl"
>>
>> http://codereview.chromium.**org/8048008/diff/1/base/bind_**
>>
helpers.h#newcode57<http://codereview.chromium.org/8048008/diff/1/base/bind_helpers.h#newcode57>
>> base/bind_helpers.h:57: //   Callback <int(void)> cb =
>> Bind(&DoSomething, 1);
>> nit: no space before "<"
>>
>> http://codereview.chromium.**org/8048008/diff/1/base/bind_**
>>
helpers.h#newcode58<http://codereview.chromium.org/8048008/diff/1/base/bind_helpers.h#newcode58>
>> base/bind_helpers.h:58: //   Closure c = IgnoreReturn(cb);  // Prints
>> "1"
>> All of the use cases I can imagine would basically look like:
>>
>>  IgnoreReturn(Bind(...))
>>
>> Would it make more sense to just have:
>>
>>  BindIgnoringReturn(...)
>>
>> Or, would that result in too much template bloat?
>
>
> (heading in...will answer other points later).
>
> I like the wrapping version a bit better since it has less bloat, but also
> because I think there's a reasonable case in the future to just remove
> IgnoreReturn, and modify the type system of Callback to specifically allow
> removing the return value.
>
> So, for any set of parameter A..., base::Callback<void(A...)> should just
> be assignable from base::Callback<R(A...)>.
>
> This is type safe because void(A...) is a supertype of R(A...).  Really,
> T(A...) is a supertype of R(A...) as long as T is a supertype of R.
>  Tr1::bind allows for these assignments, and adding support in for us isn't
> super hard.  I had just avoided it initially because I wanted to make sure
> we had a need for it first.
>
> -Albert
>

[darin (slow to review), 2011-09-27 17:31:50]

On Tue, Sep 27, 2011 at 9:55 AM, Albert J. Wong (王重傑)
<ajwong@chromium.org>wrote:

>
>
> On Tue, Sep 27, 2011 at 9:48 AM, <darin@chromium.org> wrote:
>
>>
>>
http://codereview.chromium.**org/8048008/diff/1/base/bind_**helpers.h<http://...
>> File base/bind_helpers.h (right):
>>
>> http://codereview.chromium.**org/8048008/diff/1/base/bind_**
>>
helpers.h#newcode56<http://codereview.chromium.org/8048008/diff/1/base/bind_helpers.h#newcode56>
>> base/bind_helpers.h:56: //   int DoSomething(int arg) { cout << arg <<
>> end; }
>> nit: "end" -> "endl"
>>
>> http://codereview.chromium.**org/8048008/diff/1/base/bind_**
>>
helpers.h#newcode57<http://codereview.chromium.org/8048008/diff/1/base/bind_helpers.h#newcode57>
>> base/bind_helpers.h:57: //   Callback <int(void)> cb =
>> Bind(&DoSomething, 1);
>> nit: no space before "<"
>>
>> http://codereview.chromium.**org/8048008/diff/1/base/bind_**
>>
helpers.h#newcode58<http://codereview.chromium.org/8048008/diff/1/base/bind_helpers.h#newcode58>
>> base/bind_helpers.h:58: //   Closure c = IgnoreReturn(cb);  // Prints
>> "1"
>> All of the use cases I can imagine would basically look like:
>>
>>  IgnoreReturn(Bind(...))
>>
>> Would it make more sense to just have:
>>
>>  BindIgnoringReturn(...)
>>
>> Or, would that result in too much template bloat?
>
>
> (heading in...will answer other points later).
>
> I like the wrapping version a bit better since it has less bloat, but also
> because I think there's a reasonable case in the future to just remove
> IgnoreReturn, and modify the type system of Callback to specifically allow
> removing the return value.
>
> So, for any set of parameter A..., base::Callback<void(A...)> should just
> be assignable from base::Callback<R(A...)>.
>
> This is type safe because void(A...) is a supertype of R(A...).  Really,
> T(A...) is a supertype of R(A...) as long as T is a supertype of R.
>  Tr1::bind allows for these assignments, and adding support in for us isn't
> super hard.  I had just avoided it initially because I wanted to make sure
> we had a need for it first.
>
> -Albert
>


OK, LGTM

I agree with Will that implicit conversions make me uneasy.

[awong, 2011-09-27 21:35:00]

comments fixed. Submitting.

http://codereview.chromium.org/8048008/diff/1/base/bind_helpers.h
File base/bind_helpers.h (right):

http://codereview.chromium.org/8048008/diff/1/base/bind_helpers.h#newcode56
base/bind_helpers.h:56: //   int DoSomething(int arg) { cout << arg << end; }
On 2011/09/27 16:48:01, darin wrote:
> nit: "end" -> "endl"

Done.

http://codereview.chromium.org/8048008/diff/1/base/bind_helpers.h#newcode57
base/bind_helpers.h:57: //   Callback <int(void)> cb = Bind(&DoSomething, 1);
On 2011/09/27 16:48:01, darin wrote:
> nit: no space before "<"

Done.