Unified callback system.

Unified callback system based on tr1::function/tr1::bind and Google's internal callback code.

This callback system allows for creation of functors for normal functions, methods, and const methods.  It is a superset of the functionality of NewRunnableMethod, NewRunnableFunction, NewCallback, and CreateFunctor.

We support partial binding of function arguments, and also specification of refcounting semantics by wrapping a target object in a wrapper object.

BUG=35223
TEST=none

[awong, 2011-01-19 04:52:35]

Okay, here's the next draft.

This should be closer to a real implementation.  Please ignore the
crazy-crazy-crazy template specialization for Invoker.  I'll fix that later to
look like this:

template<typename R, typename Functor, typename BoundArgs, typename FreeArgs>
Invoker;

And let BoundArgs/FreeArgs be a tuple type (even if we don't bother storing
things as tuples).

[awong, 2011-01-19 04:59:36]

To be clear, right now, I'm looking mainly for feedback about the basic
structure of the classes.

The code as-is likely doesn't compile.  Also I'd like to avoid wordsmithing
the comments, or fixing style until after something works.

FYI, this is just the shell of the system at this point. We're obviously
missing features such as the unretained and const_ref wrappers.

-Albert


On Tue, Jan 18, 2011 at 8:52 PM, <ajwong@chromium.org> wrote:

> Reviewers: darin, brettw, willchan,
>
> Message:
> Okay, here's the next draft.
>
> This should be closer to a real implementation.  Please ignore the
> crazy-crazy-crazy template specialization for Invoker.  I'll fix that later
> to
> look like this:
>
> template<typename R, typename Functor, typename BoundArgs, typename
> FreeArgs>
> Invoker;
>
> And let BoundArgs/FreeArgs be a tuple type (even if we don't bother storing
> things as tuples).
>
> Description:
> Type-system for unified callbacks based on tr1::function's API.
>
> BUG=35223
> TEST=none
>
> Please review this at http://codereview.chromium.org/6109007/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/base
>
> Affected files:
>  M base/callback.h
>  A base/prebind.h
>
>
>

[willchan no longer on Chromium, 2011-01-19 16:57:43]

Resisted the urge to nit everything. Here are just the high level comments. I
spent a lot of time thinking about the whole PolymorphicInvoke thing too.
Something about it annoys me, but I can't pin down what it is. I guess I'm fine
with it.

http://codereview.chromium.org/6109007/diff/2001/base/callback.h
File base/callback.h (right):

http://codereview.chromium.org/6109007/diff/2001/base/callback.h#newcode283
base/callback.h:283: // rejected because of the number of copy constructors
invocations when binding arguments.
I'm not trying to wordsmith the comments here, just want to note this down for
the future.

Perhaps we should do tr1 justice and note that it's because of lack of rvalue
references, which will be fixed in C++0x. We can't wait for that though, and
it's just an implementation detail.

http://codereview.chromium.org/6109007/diff/2001/base/callback.h#newcode299
base/callback.h:299: class InvokerBase : public
RefCountedThreadSafe<InvokerBase> {
Just noting that this is RefCountedThreadSafe. Every time we pass this results
in a few atomic operations. Do we want to encourage passing Callbacks to
functions by const reference instead of copy by value? That'd help minimize the
atomics. Did we discuss this in the meeting? If so, I forget and am sorry to
recover old ground. Note that if we wanted to pass via const reference, then
Run() would have to be a const member function, which is fine by me, but maybe
others find it weird.

This invoker stuff definitely should not be contaminating the base namespace.
Remember to stuff it in base::internal later.

http://codereview.chromium.org/6109007/diff/2001/base/callback.h#newcode301
base/callback.h:301: virtual ~InvokerBase() {}
Why is this virtual?

http://codereview.chromium.org/6109007/diff/2001/base/prebind.h
File base/prebind.h (right):

http://codereview.chromium.org/6109007/diff/2001/base/prebind.h#newcode42
base/prebind.h:42: typedef InvokerBase<R(X0)(P0)> SelfType;
I'm frankly a bit confused why this is InvokerBase<...> rather than
Invoker<...>.

http://codereview.chromium.org/6109007/diff/2001/base/prebind.h#newcode110
base/prebind.h:110: return unwrapped->f_(unwrapped->p0_, const_cast<A1&>(a1));
I'm trying to grok everything, but this const_cast<> looks like it's probably
caused by an implementation error. I don't believe this should be necessary. Is
it? I need to think about it more.

[awong, 2011-01-20 20:27:31]

Thanks for the restraint on nitpicking. :)

I'll try to get a new version out next week.  I responded to your comments
preemptively though.

As for the "PolymorphicInvoke", yeah, that feels dirty to me as well.

An alternative is to create a set of base classes for each arity of unbound
parameters, and then have each Callback/Invoke pair store/inherit from the
correct one. Then use virtual dispatch to "do the right thing."  We pay the
virtual dispatch versus I think in the current method, we're actually slightly
faster.  But that's micro-optimizing early.

FYI, this style of type-erasure is similar to how gcc's tr1::bind does it.

http://codereview.chromium.org/6109007/diff/2001/base/callback.h
File base/callback.h (right):

http://codereview.chromium.org/6109007/diff/2001/base/callback.h#newcode283
base/callback.h:283: // rejected because of the number of copy constructors
invocations when binding arguments.
On 2011/01/19 16:57:43, willchan wrote:
> I'm not trying to wordsmith the comments here, just want to note this down for
> the future.
> 
> Perhaps we should do tr1 justice and note that it's because of lack of rvalue
> references, which will be fixed in C++0x. We can't wait for that though, and
> it's just an implementation detail.

Ooh...good point.  Will add a note about it.

http://codereview.chromium.org/6109007/diff/2001/base/callback.h#newcode299
base/callback.h:299: class InvokerBase : public
RefCountedThreadSafe<InvokerBase> {
On 2011/01/19 16:57:43, willchan wrote:
> Just noting that this is RefCountedThreadSafe. Every time we pass this results
> in a few atomic operations. Do we want to encourage passing Callbacks to
> functions by const reference instead of copy by value? That'd help minimize
the
> atomics. Did we discuss this in the meeting? If so, I forget and am sorry to
> recover old ground. Note that if we wanted to pass via const reference, then
> Run() would have to be a const member function, which is fine by me, but maybe
> others find it weird.
> 
> This invoker stuff definitely should not be contaminating the base namespace.
> Remember to stuff it in base::internal later.

I think we started talking about it, but never actually came to a conclusion. 
I'll add a TODO to consider on the next pass.  Would like to punt though until
basic functionality is going.  The code is complex enough as it is. :(

http://codereview.chromium.org/6109007/diff/2001/base/callback.h#newcode301
base/callback.h:301: virtual ~InvokerBase() {}
On 2011/01/19 16:57:43, willchan wrote:
> Why is this virtual?

The scope_refptr's static type is scoped_refptr<InvokerBase>...which I think
means it will call InvokerBase's destructor.  However, the pointer is referring
to a child, so we need this to be virtual to ensure that the most-derived
class's destructor is called.

Am I wrong about this?

http://codereview.chromium.org/6109007/diff/2001/base/prebind.h
File base/prebind.h (right):

http://codereview.chromium.org/6109007/diff/2001/base/prebind.h#newcode42
base/prebind.h:42: typedef InvokerBase<R(X0)(P0)> SelfType;
On 2011/01/19 16:57:43, willchan wrote:
> I'm frankly a bit confused why this is InvokerBase<...> rather than
> Invoker<...>.


Errr...yes, that's just a mistake.  I uploaded without fully getting it to
compile since I ran out of time to work on it. :(

http://codereview.chromium.org/6109007/diff/2001/base/prebind.h#newcode110
base/prebind.h:110: return unwrapped->f_(unwrapped->p0_, const_cast<A1&>(a1));
On 2011/01/19 16:57:43, willchan wrote:
> I'm trying to grok everything, but this const_cast<> looks like it's probably
> caused by an implementation error. I don't believe this should be necessary.
Is
> it? I need to think about it more.


Actually, I do think it's necessary.  If A1 == "int*", then the variable a1
actually has the type "const int*&" which I think means that if you pass to
"unwrapped->f_", you will get a const conversion violation.

The const_cast<> is actually returning a1 back to the original constness of the
function.

You need "const A1&" because if it's just "A1&", and you pass a literal (eg.,
int 1), you will get a warning.

I need to unittest the edge cases to be sure, but I think this is the correct
solution....sounds like it needs a comment though.

[willchan no longer on Chromium, 2011-01-21 02:30:12]

http://codereview.chromium.org/6109007/diff/2001/base/callback.h
File base/callback.h (right):

http://codereview.chromium.org/6109007/diff/2001/base/callback.h#newcode301
base/callback.h:301: virtual ~InvokerBase() {}
On 2011/01/20 20:27:35, awong wrote:
> On 2011/01/19 16:57:43, willchan wrote:
> > Why is this virtual?
> 
> The scope_refptr's static type is scoped_refptr<InvokerBase>...which I think
> means it will call InvokerBase's destructor.  However, the pointer is
referring
> to a child, so we need this to be virtual to ensure that the most-derived
> class's destructor is called.
> 
> Am I wrong about this?

Yes, I'm wrong. I wrote that comment early on when I was still grokking how this
all worked.

http://codereview.chromium.org/6109007/diff/2001/base/prebind.h
File base/prebind.h (right):

http://codereview.chromium.org/6109007/diff/2001/base/prebind.h#newcode110
base/prebind.h:110: return unwrapped->f_(unwrapped->p0_, const_cast<A1&>(a1));
On 2011/01/20 20:27:35, awong wrote:
> On 2011/01/19 16:57:43, willchan wrote:
> > I'm trying to grok everything, but this const_cast<> looks like it's
probably
> > caused by an implementation error. I don't believe this should be necessary.
> Is
> > it? I need to think about it more.
> 
> 
> Actually, I do think it's necessary.  If A1 == "int*", then the variable a1
> actually has the type "const int*&" which I think means that if you pass to
> "unwrapped->f_", you will get a const conversion violation.
> 
> The const_cast<> is actually returning a1 back to the original constness of
the
> function.
> 
> You need "const A1&" because if it's just "A1&", and you pass a literal (eg.,
> int 1), you will get a warning.
> 
> I need to unittest the edge cases to be sure, but I think this is the correct
> solution....sounds like it needs a comment though.

Hmmmm, I'm not sure this works. How does it handle the case with:

const int kFooArray[] = { 1, 2, 3 };

void Foo(int* array) { array[0] = 999; }

AFAICT, calling Run(kFooArray) will cause A1 == int*, and when Foo() is invoked,
it'll try to modify kFooArray[0], which may be in the readonly data segment, and
SIGSEGV.

It sounds like you're trying to have Run() always take reference arguments in
order to optimize their calls. But this will also create inefficiencies for most
POD types (except large structs), which should just be copied by value.

I have two proposals here.

(1) If the caller wants Run() to be efficient, then have him/her use
std::tr1::cref() to pass it by reference. Use PolymorphicInvoke(InvokerBase*,
A1);
(2) Add a Invoker specialization for POD types (yes, this require multiply the
templates required). Specialize for is_pod<A1>::value. For true, have
PolymorphicInvoke(InvokerBase*, A1). For false, have
PolymorphicInvoke(InvokerBase*, A1&).

I'm open to other options, those are just the ones that immediately come to
mind.

[awong, 2011-01-21 12:00:36]

I made another attempt at it.

Two goals here:

  1) Reduce the number of specializations for Prebind.  Basically use "typename
Sig" rather than a broken out function signature, then pass off the type-erasure
code into Callback<>.
  2) See if it would be cleaner looking if I broke apart the different dimension
that we need to specialize the callbacks on.

For #2, it is a wash in terms of the number of classes I would need to write.

So, to support prebinding for up to 5-arity functions, there must be 15 invokers
(5+4+3+2+1).  Because C++ differentiates between methods, const-methods, and
functions, we need 3 sets of these making for 45 specializations of the Invoker
class.

I was hoping to be cleaver and reduce the number of classes, but that didn't
work.  In the end I came up with:

  15 function type-traits classes
  15 polymorphic invoke classes
  5 invoker storage classes
  10 member/const-method -> normal function call syntax adapter classes.

Which is back to 45 classes.

I think having everything just expanded out is actually easier to understand.

Any opinion on this will?  If not, I'm going to go for it on the next pass.

http://codereview.chromium.org/6109007/diff/2001/base/prebind.h
File base/prebind.h (right):

http://codereview.chromium.org/6109007/diff/2001/base/prebind.h#newcode110
base/prebind.h:110: return unwrapped->f_(unwrapped->p0_, const_cast<A1&>(a1));
Ah, I see what you're saying now.  Because I used a const A&, the compiler will
allows a const object to be passed...and then I const_cast it into the wrong
type.  Good catch.

Suggestion #2 sounds the nicest.

For now, I'll revert to a copy and add a TODO.

On 2011/01/21 02:30:12, willchan wrote:
> On 2011/01/20 20:27:35, awong wrote:
> > On 2011/01/19 16:57:43, willchan wrote:
> > > I'm trying to grok everything, but this const_cast<> looks like it's
> probably
> > > caused by an implementation error. I don't believe this should be
necessary.
> > Is
> > > it? I need to think about it more.
> > 
> > 
> > Actually, I do think it's necessary.  If A1 == "int*", then the variable a1
> > actually has the type "const int*&" which I think means that if you pass to
> > "unwrapped->f_", you will get a const conversion violation.
> > 
> > The const_cast<> is actually returning a1 back to the original constness of
> the
> > function.
> > 
> > You need "const A1&" because if it's just "A1&", and you pass a literal
(eg.,
> > int 1), you will get a warning.
> > 
> > I need to unittest the edge cases to be sure, but I think this is the
correct
> > solution....sounds like it needs a comment though.
> 
> Hmmmm, I'm not sure this works. How does it handle the case with:
> 
> const int kFooArray[] = { 1, 2, 3 };
> 
> void Foo(int* array) { array[0] = 999; }
> 
> AFAICT, calling Run(kFooArray) will cause A1 == int*, and when Foo() is
invoked,
> it'll try to modify kFooArray[0], which may be in the readonly data segment,
and
> SIGSEGV.
>
> It sounds like you're trying to have Run() always take reference arguments in
> order to optimize their calls. But this will also create inefficiencies for
most
> POD types (except large structs), which should just be copied by value.
> 
> I have two proposals here.
> 
> (1) If the caller wants Run() to be efficient, then have him/her use
> std::tr1::cref() to pass it by reference. Use PolymorphicInvoke(InvokerBase*,
> A1);
> (2) Add a Invoker specialization for POD types (yes, this require multiply the
> templates required). Specialize for is_pod<A1>::value. For true, have
> PolymorphicInvoke(InvokerBase*, A1). For false, have
> PolymorphicInvoke(InvokerBase*, A1&).
> 
> I'm open to other options, those are just the ones that immediately come to
> mind.

[awong, 2011-01-29 08:20:30]

Here's a 90% working system including:
  - Refcounts the first object
  - Unwrapped and ConstRef wrappers
  - Support for wrapping functions, methods, and const method
  - Has a "StorageWrapper" that makes it impossible to leak from Prebind
  - Some rudimentary unittests

What's not there yet:
  - Full set of unittest & no compile tests
  - compiletime asserts to for enforcing that Unretained can't be 
     used on a refcounted object.
  - Any attempt to optimize away argument copying, or inlining
     functions (that's the next step)
  - Filling out up beyond 2 prebound arguments.


This would be a good time to give the implementation architecture a scrub. 
Otherwise, I'm going to try and optimize the argument passing, then expand it to
6 arguments (and end up wasting a lot of typing if there are any changes).

[awong, 2011-01-29 08:20:33]

Here's a 90% working system including:
  - Refcounts the first object
  - Unwrapped and ConstRef wrappers
  - Support for wrapping functions, methods, and const method
  - Has a "StorageWrapper" that makes it impossible to leak from Prebind
  - Some rudimentary unittests

What's not there yet:
  - Full set of unittest & no compile tests
  - compiletime asserts to for enforcing that Unretained can't be 
     used on a refcounted object.
  - Any attempt to optimize away argument copying, or inlining
     functions (that's the next step)
  - Filling out up beyond 2 prebound arguments.


This would be a good time to give the implementation architecture a scrub. 
Otherwise, I'm going to try and optimize the argument passing, then expand it to
6 arguments (and end up wasting a lot of typing if there are any changes).

[darin (slow to review), 2011-02-01 00:35:56]

it would be good to see more samples in the unit tests...  i think this is off
to a really great start!

http://codereview.chromium.org/6109007/diff/22001/base/callback.h
File base/callback.h (right):

http://codereview.chromium.org/6109007/diff/22001/base/callback.h#newcode318
base/callback.h:318: // compiler that ther template only have 1 type parameter,
the function
need to proof read comments.  there's a variety of typos.

http://codereview.chromium.org/6109007/diff/22001/base/prebind.h
File base/prebind.h (right):

http://codereview.chromium.org/6109007/diff/22001/base/prebind.h#newcode17
base/prebind.h:17: class UnretainedWrapper {
it seems like it would be helpful to put classes like this which are not meant
to be used directly into the "internal" namespace.  that way you make it clearer
which of these things are for direct, public consumption.

as with "public:" and "private:" class sections, it might be nice to group all
of the internal things together.  an internal namespace helps do that.  then at
the bottom of the file, you can just declare the public things, and focus on
documenting those for the end-user.

http://codereview.chromium.org/6109007/diff/22001/base/prebind.h#newcode42
base/prebind.h:42: T unwrap(T o) { return o; }
why is it "unwrap" and not "Unwrap"?

http://codereview.chromium.org/6109007/diff/22001/base/prebind.h#newcode48
base/prebind.h:48: const T* unwrap(ConstRefWrapper<T> const_ref) { return
const_ref.get(); }
why are Unretained, ConstRef, and unwrap defined in a header file named
prebind.h?  i'm struggling to make the connection.

although perhaps they are so little code that including them here is fine since
they are needed here.  i'm just not sure that i would think to include this
header if i needed one of those functions w/o the Prebind function.

http://codereview.chromium.org/6109007/diff/22001/base/prebind.h#newcode52
base/prebind.h:52: template <bool ref, typename O>
"O" seems like a poor typename because it looks too much like a zero :)  If you
want a one letter type, how about the canonical "T"?

http://codereview.chromium.org/6109007/diff/22001/base/prebind.h#newcode190
base/prebind.h:190: InvokerStorageHolder<T> MakeInvokerStorageHolder(T* o) {
shouldn't this go in callback.h near the InvokerStorageHolder definition?

[scherkus (not reviewing), 2011-02-01 01:00:09]

http://codereview.chromium.org/6109007/diff/22001/base/callback_unittest.cc
File base/callback_unittest.cc (right):

http://codereview.chromium.org/6109007/diff/22001/base/callback_unittest.cc#n...
base/callback_unittest.cc:8: #include "base/logging.h"
include order

http://codereview.chromium.org/6109007/diff/22001/base/callback_unittest.cc#n...
base/callback_unittest.cc:72: int n;
move this to private section?

http://codereview.chromium.org/6109007/diff/22001/base/callback_unittest.cc#n...
base/callback_unittest.cc:146: #if 0
:)

[awong, 2011-02-01 01:21:37]

Seems like people are generally happy with the structure.

I'm currently porting the code and unittest to a pump-based script.  This should
allow me to make some very thorough unittests.

Then I'll fix the namespacing and comments and reupload.

http://codereview.chromium.org/6109007/diff/22001/base/callback.h
File base/callback.h (right):

http://codereview.chromium.org/6109007/diff/22001/base/callback.h#newcode318
base/callback.h:318: // compiler that ther template only have 1 type parameter,
the function
On 2011/02/01 00:35:56, darin wrote:
> need to proof read comments.  there's a variety of typos.

Yep...will revisit all comments on the next version.

http://codereview.chromium.org/6109007/diff/22001/base/callback_unittest.cc
File base/callback_unittest.cc (right):

http://codereview.chromium.org/6109007/diff/22001/base/callback_unittest.cc#n...
base/callback_unittest.cc:8: #include "base/logging.h"
On 2011/02/01 01:00:10, scherkus wrote:
> include order

Will fix in next pass...I'm about to regenerate these files based on pump, so
they're going to look pretty different.

http://codereview.chromium.org/6109007/diff/22001/base/callback_unittest.cc#n...
base/callback_unittest.cc:72: int n;
On 2011/02/01 01:00:10, scherkus wrote:
> move this to private section?

Yep. This will get recreated by pump in a bit.

http://codereview.chromium.org/6109007/diff/22001/base/callback_unittest.cc#n...
base/callback_unittest.cc:146: #if 0
On 2011/02/01 01:00:10, scherkus wrote:
> :)

*sigh*  Yeah, I wish I had a better method for this.

http://codereview.chromium.org/6109007/diff/22001/base/prebind.h
File base/prebind.h (right):

http://codereview.chromium.org/6109007/diff/22001/base/prebind.h#newcode17
base/prebind.h:17: class UnretainedWrapper {
On 2011/02/01 00:35:56, darin wrote:
> it seems like it would be helpful to put classes like this which are not meant
> to be used directly into the "internal" namespace.  that way you make it
clearer
> which of these things are for direct, public consumption.
> 
> as with "public:" and "private:" class sections, it might be nice to group all
> of the internal things together.  an internal namespace helps do that.  then
at
> the bottom of the file, you can just declare the public things, and focus on
> documenting those for the end-user.

Yep...will do that in next iteration.  Was avoiding cause I kept churning the
code so much.

http://codereview.chromium.org/6109007/diff/22001/base/prebind.h#newcode42
base/prebind.h:42: T unwrap(T o) { return o; }
On 2011/02/01 00:35:56, darin wrote:
> why is it "unwrap" and not "Unwrap"?

More typoing.  I haven't actually scrubbed this for typos or understandability
yet.  Will do on next pass.

http://codereview.chromium.org/6109007/diff/22001/base/prebind.h#newcode48
base/prebind.h:48: const T* unwrap(ConstRefWrapper<T> const_ref) { return
const_ref.get(); }
On 2011/02/01 00:35:56, darin wrote:
> why are Unretained, ConstRef, and unwrap defined in a header file named
> prebind.h?  i'm struggling to make the connection.
> 
> although perhaps they are so little code that including them here is fine
since
> they are needed here.  i'm just not sure that i would think to include this
> header if i needed one of those functions w/o the Prebind function.

I feel like they actually belong here.  They are purely used for use in
conjunction with Prebind.  What else would you use them for?

http://codereview.chromium.org/6109007/diff/22001/base/prebind.h#newcode52
base/prebind.h:52: template <bool ref, typename O>
On 2011/02/01 00:35:56, darin wrote:
> "O" seems like a poor typename because it looks too much like a zero :)  If
you
> want a one letter type, how about the canonical "T"?

Yeah, that seems better.  Will change.

http://codereview.chromium.org/6109007/diff/22001/base/prebind.h#newcode190
base/prebind.h:190: InvokerStorageHolder<T> MakeInvokerStorageHolder(T* o) {
On 2011/02/01 00:35:56, darin wrote:
> shouldn't this go in callback.h near the InvokerStorageHolder definition?

Yes....will move.

[akalin, 2011-02-01 11:29:26]

Finally had some time to play with this!  I like it. :)

FWIW, I think polymorphic_invoke_ is the best way to do that.

http://codereview.chromium.org/6109007/diff/22003/base/callback_unittest.cc
File base/callback_unittest.cc (right):

http://codereview.chromium.org/6109007/diff/22003/base/callback_unittest.cc#n...
base/callback_unittest.cc:119: func = base::Prebind(&RefFoo::one_arg, &f);
you may already know this, but you could overload operator= for Callback to
eliminate the temporary Callback created here (and copy)

http://codereview.chromium.org/6109007/diff/22003/base/callback_unittest.cc#n...
base/callback_unittest.cc:147: TEST(Callback, Prebind_one_arg_nocompile) {
Just to be clear, are these two functions supposed to compile but currently
don't?

http://codereview.chromium.org/6109007/diff/22003/base/uber_callback.h
File base/uber_callback.h (right):

http://codereview.chromium.org/6109007/diff/22003/base/uber_callback.h#newcod...
base/uber_callback.h:105:
const_cast<InvokerStorageHolder<T>&>(invoker_holder).invoker_storage_);
I don't like this const_cast, although I understand why it's here.  I notice
that changing the argument to be of type InvokeStorageHolder<T> instead of a
const ref seems to still work and not cause any extra copies, based on some
gdb'ing.  I'm not sure what magic we'd rely upon for that, though.

[awong, 2011-02-01 23:20:45]

Responses to Fred's comments.

On the current TODO list:
  1) fix unittests
  2) scrub the comments in callback.h and prebind.h
  3) Add an assignment operator for callback
  4) Make sure this compiles on windows.

Then I think it's ready for initial checkin.  TODO is still to optimize the
argument forwarding in the Run() method.

http://codereview.chromium.org/6109007/diff/22003/base/callback_unittest.cc
File base/callback_unittest.cc (right):

http://codereview.chromium.org/6109007/diff/22003/base/callback_unittest.cc#n...
base/callback_unittest.cc:119: func = base::Prebind(&RefFoo::one_arg, &f);
On 2011/02/01 11:29:27, akalin wrote:
> you may already know this, but you could overload operator= for Callback to
> eliminate the temporary Callback created here (and copy)

Oh....I actually forgot about that. Good point.  I should do that.

http://codereview.chromium.org/6109007/diff/22003/base/callback_unittest.cc#n...
base/callback_unittest.cc:147: TEST(Callback, Prebind_one_arg_nocompile) {
On 2011/02/01 11:29:27, akalin wrote:
> Just to be clear, are these two functions supposed to compile but currently
> don't?

No, these are funcitons that I want explicitly to not compile.  About to redo
the unittests to be more readable and will add a nice comment then.

There's a few features from tr1::bind that I'm explicitly going to disallow
until someone screams. In particular, I want

Callback<void(void)> c;
Callback<int(void)> r_c;
c = r_c;  // This should fail.  It works in tr1.

Also, full currying is currently not implemented. Didn't seem to be worth its
weight in templates.

http://codereview.chromium.org/6109007/diff/22003/base/uber_callback.h
File base/uber_callback.h (right):

http://codereview.chromium.org/6109007/diff/22003/base/uber_callback.h#newcod...
base/uber_callback.h:105:
const_cast<InvokerStorageHolder<T>&>(invoker_holder).invoker_storage_);
On 2011/02/01 11:29:27, akalin wrote:
> I don't like this const_cast, although I understand why it's here.  I notice
> that changing the argument to be of type InvokeStorageHolder<T> instead of a
> const ref seems to still work and not cause any extra copies, based on some
> gdb'ing.  I'm not sure what magic we'd rely upon for that, though.

I personally prefer the explicitness of the const_cast() in this case.  I betcha
we're getting sort of weird optimization cause we're pass in a temporary.

Do you happen to have the assembly of the two calls?  What optimization level
were you using?

BTW, I'm thinking of doing another const_cast down in the Run() functions for
each of the arguments....

[akalin, 2011-02-03 04:32:48]

On 2011/02/01 23:20:45, awong wrote:
> Also, full currying is currently not implemented. Didn't seem to be worth its
> weight in templates.

Do you mean that if you have int foo(int x, int y, int z), it wouldn't be
possible to do callback<int(int)> c = Prebind(foo, 5, 6), and then c.Run(7)?  If
so, I thought one of the reasons we wanted to revamp the callback library was to
support this?

> I personally prefer the explicitness of the const_cast() in this case.  I
betcha
> we're getting sort of weird optimization cause we're pass in a temporary.

After thinking about it a bit more, I think what bothers me is that the return
type of Prebind() is different from that of callback<>, even though the common
case is something like "callback<...> foo = Prebind(blah)".  I think this is
necessary because the return value of Prebind() differs depending on its
arguments, and we want to erase everything but the essential callback type,
which we do when we assign to a callback variable.  But it does open the door to
stuff like the following not working:

InvokerStorage<T> foo = Prebind(...);
callback<...> bar = foo;
callback<...> baz = foo; // doesn't do what you expect!

I don't know if this is something worth worrying about.  What do you think?

[awong, 2011-02-03 04:40:45]

On Wed, Feb 2, 2011 at 8:32 PM, <akalin@chromium.org> wrote:

> On 2011/02/01 23:20:45, awong wrote:
>
>> Also, full currying is currently not implemented. Didn't seem to be worth
>> its
>> weight in templates.
>>
>
> Do you mean that if you have int foo(int x, int y, int z), it wouldn't be
> possible to do callback<int(int)> c = Prebind(foo, 5, 6), and then
> c.Run(7)?  If
> so, I thought one of the reasons we wanted to revamp the callback library
> was to
> support this?


No...that case works.  We just can't keep currying easily.  So  building off
of above, I couldn't then do

Callback<int(void)> c_bound = Prebind(c, 7);  // This won't compile.

I could fix it, basically the "Callback" class would be considered a whole
other variant on function...and then we're entering multiple forwarding,
etc. etc.



>  I personally prefer the explicitness of the const_cast() in this case.  I
>>
> betcha
>
>> we're getting sort of weird optimization cause we're pass in a temporary.
>>
>
> After thinking about it a bit more, I think what bothers me is that the
> return
> type of Prebind() is different from that of callback<>, even though the
> common
> case is something like "callback<...> foo = Prebind(blah)".  I think this
> is
> necessary because the return value of Prebind() differs depending on its
> arguments, and we want to erase everything but the essential callback type,
> which we do when we assign to a callback variable.  But it does open the
> door to
> stuff like the following not working:
>
> InvokerStorage<T> foo = Prebind(...);
> callback<...> bar = foo;
> callback<...> baz = foo; // doesn't do what you expect!
>
> I don't know if this is something worth worrying about.  What do you think?


Yeah...I thought about that.  I didn't have much of a better idea.  You hit
the nail on the head that the return type must differ.

It is possible to make Prebind return the actually Callback<> type.  But to
do that, I would introduce as many Callback<> templates as I have
FunctionTraits() templates because I have to unpack the Sig type inorder to
solve-out the return type. In particular, you can't figure out the arity and
the parameter types.

Since the return type is in the internal namespace, I think it's okay.
 People woudl have to ignore that to do what you wrote above.

[akalin, 2011-02-03 04:49:26]

On 2011/02/03 04:40:45, awong wrote:
> No...that case works.  We just can't keep currying easily.  So  building off
> of above, I couldn't then do
> 
> Callback<int(void)> c_bound = Prebind(c, 7);  // This won't compile.
> 
> I could fix it, basically the "Callback" class would be considered a whole
> other variant on function...and then we're entering multiple forwarding,
> etc. etc.

Oh, okay I see.  That's fine, if anyone really wanted that they could just do
Prebind(&Callback<...>::Run, c, 7) anyway.

> Yeah...I thought about that.  I didn't have much of a better idea.  You hit
> the nail on the head that the return type must differ.
> 
> Since the return type is in the internal namespace, I think it's okay.
>  People woudl have to ignore that to do what you wrote above.

Yeah, okay.  I think in that case I'm fine with const_cast, as long as there's a
comment somewhere explaining it.

[awong, 2011-02-03 04:59:53]

On Wed, Feb 2, 2011 at 8:49 PM, <akalin@chromium.org> wrote:

> On 2011/02/03 04:40:45, awong wrote:
>
>> No...that case works.  We just can't keep currying easily.  So  building
>> off
>> of above, I couldn't then do
>>
>
>  Callback<int(void)> c_bound = Prebind(c, 7);  // This won't compile.
>>
>
>  I could fix it, basically the "Callback" class would be considered a whole
>> other variant on function...and then we're entering multiple forwarding,
>> etc. etc.
>>
>
> Oh, okay I see.  That's fine, if anyone really wanted that they could just
> do
> Prebind(&Callback<...>::Run, c, 7) anyway.
>
>
>  Yeah...I thought about that.  I didn't have much of a better idea.  You
>> hit
>> the nail on the head that the return type must differ.
>>
>
>  Since the return type is in the internal namespace, I think it's okay.
>>  People woudl have to ignore that to do what you wrote above.
>>
>
> Yeah, okay.  I think in that case I'm fine with const_cast, as long as
> there's a
> comment somewhere explaining it.


I made it mutable instead and got rid of the cast. One poison for another.
:P

[awong, 2011-02-03 05:04:18]

Updated with a slew of unittests.  Found 4 actual issues that I need to fix.

The unittests are an interesting read cause it lists all the cases that
*explicitly want to disallow*.

I also changed the argument forwarding to use const references which drops us
down to only 1 copy of the argument when using Run().

Next up, fix those bugs and then add comments.

[willchan no longer on Chromium, 2011-02-03 05:18:07]

I apologize for not looking at the changelist yet. I will look tomorrow.

On Wed, Feb 2, 2011 at 9:04 PM, <ajwong@chromium.org> wrote:

> Updated with a slew of unittests.  Found 4 actual issues that I need to
> fix.
>
> The unittests are an interesting read cause it lists all the cases that
> *explicitly want to disallow*.
>
> I also changed the argument forwarding to use const references which drops
> us
> down to only 1 copy of the argument when using Run().
>
> Next up, fix those bugs and then add comments.
>
>
> http://codereview.chromium.org/6109007/
>

[awong, 2011-02-04 04:33:16]

I think it's ready for a real review now...comments, indentation, and all.

I'm still working out a couple of compiler compat issues, but that should be
smallish.


On 2011/02/03 05:18:07, willchan wrote:
> I apologize for not looking at the changelist yet. I will look tomorrow.
> 
> On Wed, Feb 2, 2011 at 9:04 PM, <mailto:ajwong@chromium.org> wrote:
> 
> > Updated with a slew of unittests.  Found 4 actual issues that I need to
> > fix.
> >
> > The unittests are an interesting read cause it lists all the cases that
> > *explicitly want to disallow*.
> >
> > I also changed the argument forwarding to use const references which drops
> > us
> > down to only 1 copy of the argument when using Run().
> >
> > Next up, fix those bugs and then add comments.
> >
> >
> > http://codereview.chromium.org/6109007/
> >

[awong, 2011-02-05 04:53:07]

Compiler compat fixed.  I <3 stackoverflow.

Ready for review.


On 2011/02/04 04:33:16, awong wrote:
> I think it's ready for a real review now...comments, indentation, and all.
> 
> I'm still working out a couple of compiler compat issues, but that should be
> smallish.
> 
> 
> On 2011/02/03 05:18:07, willchan wrote:
> > I apologize for not looking at the changelist yet. I will look tomorrow.
> > 
> > On Wed, Feb 2, 2011 at 9:04 PM, <mailto:ajwong@chromium.org> wrote:
> > 
> > > Updated with a slew of unittests.  Found 4 actual issues that I need to
> > > fix.
> > >
> > > The unittests are an interesting read cause it lists all the cases that
> > > *explicitly want to disallow*.
> > >
> > > I also changed the argument forwarding to use const references which drops
> > > us
> > > down to only 1 copy of the argument when using Run().
> > >
> > > Next up, fix those bugs and then add comments.
> > >
> > >
> > > http://codereview.chromium.org/6109007/
> > >

[willchan no longer on Chromium, 2011-02-06 10:26:49]

Sorry, not done yet. Still need to keep looking, but I'm going to bed.

http://codereview.chromium.org/6109007/diff/27010/base/uber_callback.h.pump
File base/uber_callback.h.pump (right):

http://codereview.chromium.org/6109007/diff/27010/base/uber_callback.h.pump#n...
base/uber_callback.h.pump:17: // New, super-duper, unified Callback system. 
This will eventually replace
Change comments. This will look silly in 2-3 years time :) I try to keep
comments referring to to the present.

http://codereview.chromium.org/6109007/diff/27010/base/uber_callback_helpers.h
File base/uber_callback_helpers.h (right):

http://codereview.chromium.org/6109007/diff/27010/base/uber_callback_helpers....
base/uber_callback_helpers.h:37: mutable scoped_refptr<InvokerStorageBase>
invoker_storage_;
in structs, you don't use the trailing underscore for members.

http://codereview.chromium.org/6109007/diff/11035/base/prebind_helpers.h
File base/prebind_helpers.h (right):

http://codereview.chromium.org/6109007/diff/11035/base/prebind_helpers.h#newc...
base/prebind_helpers.h:251:
COMPILE_ASSERT(internal::SupportsAddRefAndRelease<T>::value == false,
I'm trying to figure out if this is more desirable than just doing
is_convertible to base::RefCounted<T> or base::RefCountedThreadSafe<T>. I don't
know how much we want to support alternative AddRef()/Release() systems in our
callback system. I don't know too much about those edge cases.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h
File base/uber_callback.h (right):

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcod...
base/uber_callback.h:202: Callback() : polymorphic_invoke_(NULL) { }
When is this ever used? Can this be private?

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcod...
base/uber_callback.h:204: typedef
R(*PolymorphicInvoke)(internal::InvokerStorageBase*);
Move the typedef before the constructor.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcod...
base/uber_callback.h:342: typedef
R(*PolymorphicInvoke)(internal::InvokerStorageBase*, const A1&,
Nit: you should probably change the pump to emit a line for each of these
arguments, so you don't have the strange formatting issues here.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcod...
base/uber_callback.h:351: R Run(const A1& a1, const A2& a2, const A3& a3, const
A4& a4, const A5& a5,
Ditto on the line per argument to avoid the formatting indentation issue.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback_helpers.h
File base/uber_callback_helpers.h (right):

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback_helpers....
base/uber_callback_helpers.h:34: mutable scoped_refptr<InvokerStorageBase>
invoker_storage_;
Why is this mutable?

[willchan no longer on Chromium, 2011-02-07 20:51:48]

Almost done now. But now it's time for lunch.

http://codereview.chromium.org/6109007/diff/11035/base/prebind.h
File base/prebind.h (right):

http://codereview.chromium.org/6109007/diff/11035/base/prebind.h#newcode35
base/prebind.h:35: // function with a fixed signature that is indepenent of
StorageType; its first
independent

http://codereview.chromium.org/6109007/diff/11035/base/prebind.h#newcode1155
base/prebind.h:1155: 
These should be private. Perhaps you need to add a Run() method to pass these
parameters.

http://codereview.chromium.org/6109007/diff/11035/base/prebind_helpers.h
File base/prebind_helpers.h (right):

http://codereview.chromium.org/6109007/diff/11035/base/prebind_helpers.h#newc...
base/prebind_helpers.h:199: struct MaybeRefcount<base::true_type, O* > {
Unnecessary whitespace after O*.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h
File base/uber_callback.h (right):

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcod...
base/uber_callback.h:207: Callback(const internal::InvokerStorageHolder<T>&
invoker_holder)
Why is this const if we're calling swap on its member variable?

[willchan no longer on Chromium, 2011-02-07 23:25:15]

http://codereview.chromium.org/6109007/diff/11035/base/prebind.h
File base/prebind.h (right):

http://codereview.chromium.org/6109007/diff/11035/base/prebind.h#newcode1276
base/prebind.h:1276: class InvokerStorage6 : public internal::InvokerStorageBase
{
You shouldn't need internal:: since you're already in the internal namespace.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h
File base/uber_callback.h (right):

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcod...
base/uber_callback.h:202: Callback() : polymorphic_invoke_(NULL) { }
On 2011/02/06 10:26:50, willchan wrote:
> When is this ever used? Can this be private?

Oops, I see now that this is the equivalent of std::tr1::function; This is fine.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcod...
base/uber_callback.h:207: Callback(const internal::InvokerStorageHolder<T>&
invoker_holder)
I just noticed this constructor is implicit. Can you change Prebind() to return
the appropriate Callback type rather than returning the
internal::InvokerStorageHolder()?

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback_helpers.h
File base/uber_callback_helpers.h (right):

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback_helpers....
base/uber_callback_helpers.h:33: }
Perhaps the destructor should DCHECK that invoker_storage_ is NULL, since you're
only ever using this to pass the concrete type to the function traits, during
which you are taking ownership of the |invoker_storage_|.

[willchan no longer on Chromium, 2011-02-07 23:30:10]

Also, other general comments:

I have a preference for Bind() over Prebind(). I believe it is a more generally
understood term. I believe base::Bind() shouldn't conflict with anything else.

And I don't want to permanently have uber_callback.h. Do you have a plan for
eventually replacing callback.h? Maybe you should just merge the uber_callback.h
code into callback.h and eventually we should have a fixit to kill off the old
uses and delete it. Or do you want to keep it separate and only once we kill off
the old one, do a global search and replace to take over callback.h?

[willchan no longer on Chromium, 2011-02-07 23:50:02]

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h
File base/uber_callback.h (right):

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcode34
base/uber_callback.h:34: // class Ref : public RefCountedThreadSafe<Ref> {
Add a public: section.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcode42
base/uber_callback.h:42: // class NoRef {
public:

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcode46
base/uber_callback.h:46: // Callback<int(void)> no_ref_cb = Prebind(&NoRef::Foo,
Unretained(&no_ref));
Please prepend the base:: namespace as needed, since this should be an example
of how most people use it, and most users won't be in the base namespace.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcode58
base/uber_callback.h:58: // Callback<int(void)> bound_ref_cb =
Prebind(&Identity, ConstRef(value));
I wonder if it's appropriate here to post a scary note about using ConstRef with
a Closure posted to the MessageLoop.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcode65
base/uber_callback.h:65: //
Include an example for Closure. I suspect that will be the most common one after
all.

[awong, 2011-02-08 18:52:26]

Addressed most comments.

I'm saving renames for the next pass.  Renames that are queued up:

   Prebind -> Bind
   ConstRef -> StoreAsConstRef
   callback.h -> callback_old.h
   uber_callback.h -> callback.h  (and #include callback_old.h)

http://codereview.chromium.org/6109007/diff/11035/base/prebind.h
File base/prebind.h (right):

http://codereview.chromium.org/6109007/diff/11035/base/prebind.h#newcode35
base/prebind.h:35: // function with a fixed signature that is indepenent of
StorageType; its first
On 2011/02/07 20:51:48, willchan wrote:
> independent

Done.

http://codereview.chromium.org/6109007/diff/11035/base/prebind.h#newcode1155
base/prebind.h:1155: 
On 2011/02/07 20:51:48, willchan wrote:
> These should be private. Perhaps you need to add a Run() method to pass these
> parameters.

:-/

Really, if I had my way, I'd make InvokerStorageN a struct.  But then you'd have
a struct inheriting from a class which emits a compiler warning.

Also, you'd have to unwrap the Sig type in order to know what the signature of
Run() would be (the arity being the hardest problem).

I added a comment explaining.

http://codereview.chromium.org/6109007/diff/11035/base/prebind.h#newcode1276
base/prebind.h:1276: class InvokerStorage6 : public internal::InvokerStorageBase
{
On 2011/02/07 23:25:15, willchan wrote:
> You shouldn't need internal:: since you're already in the internal namespace.

Done.

http://codereview.chromium.org/6109007/diff/11035/base/prebind_helpers.h
File base/prebind_helpers.h (right):

http://codereview.chromium.org/6109007/diff/11035/base/prebind_helpers.h#newc...
base/prebind_helpers.h:199: struct MaybeRefcount<base::true_type, O* > {
On 2011/02/07 20:51:48, willchan wrote:
> Unnecessary whitespace after O*.

Done.

http://codereview.chromium.org/6109007/diff/11035/base/prebind_helpers.h#newc...
base/prebind_helpers.h:251:
COMPILE_ASSERT(internal::SupportsAddRefAndRelease<T>::value == false,
On 2011/02/06 10:26:50, willchan wrote:
> I'm trying to figure out if this is more desirable than just doing
> is_convertible to base::RefCounted<T> or base::RefCountedThreadSafe<T>. I
don't
> know how much we want to support alternative AddRef()/Release() systems in our
> callback system. I don't know too much about those edge cases.

So...I recall from the meeting about this a while back that Darin expressed an
interested in wanting to inspect for AddRef/Release.

I'm not sure which is better either.

http://codereview.chromium.org/6109007/diff/11035/base/prebind_unittest.cc
File base/prebind_unittest.cc (right):

http://codereview.chromium.org/6109007/diff/11035/base/prebind_unittest.cc#ne...
base/prebind_unittest.cc:396: TEST_F(PrebindTest, Unretained) {
Need to find a way to make Unretained seem like a peer to StoreAsConstRef.

http://codereview.chromium.org/6109007/diff/11035/base/prebind_unittest.cc#ne...
base/prebind_unittest.cc:417: TEST_F(PrebindTest, ConstRef) {
Rename ConstRef to StoreAsConstRef.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h
File base/uber_callback.h (right):

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcode34
base/uber_callback.h:34: // class Ref : public RefCountedThreadSafe<Ref> {
On 2011/02/07 23:50:02, willchan wrote:
> Add a public: section.

Done.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcode42
base/uber_callback.h:42: // class NoRef {
On 2011/02/07 23:50:02, willchan wrote:
> public:

Done.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcode46
base/uber_callback.h:46: // Callback<int(void)> no_ref_cb = Prebind(&NoRef::Foo,
Unretained(&no_ref));
On 2011/02/07 23:50:02, willchan wrote:
> Please prepend the base:: namespace as needed, since this should be an example
> of how most people use it, and most users won't be in the base namespace.

Done.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcode58
base/uber_callback.h:58: // Callback<int(void)> bound_ref_cb =
Prebind(&Identity, ConstRef(value));
On 2011/02/07 23:50:02, willchan wrote:
> I wonder if it's appropriate here to post a scary note about using ConstRef
with
> a Closure posted to the MessageLoop.

Scary note added.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcode65
base/uber_callback.h:65: //
On 2011/02/07 23:50:02, willchan wrote:
> Include an example for Closure. I suspect that will be the most common one
after
> all.

Good call.  I added a couple.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcod...
base/uber_callback.h:202: Callback() : polymorphic_invoke_(NULL) { }
On 2011/02/07 23:25:15, willchan wrote:
> On 2011/02/06 10:26:50, willchan wrote:
> > When is this ever used? Can this be private?
> 
> Oops, I see now that this is the equivalent of std::tr1::function; This is
fine.

Yeah...I initially left it out too.  The my unittests told me to do otherwise. 
Yay for unittests!

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcod...
base/uber_callback.h:204: typedef
R(*PolymorphicInvoke)(internal::InvokerStorageBase*);
On 2011/02/06 10:26:50, willchan wrote:
> Move the typedef before the constructor.

Done.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcod...
base/uber_callback.h:207: Callback(const internal::InvokerStorageHolder<T>&
invoker_holder)
On 2011/02/07 23:25:15, willchan wrote:
> I just noticed this constructor is implicit. Can you change Prebind() to
return
> the appropriate Callback type rather than returning the
> internal::InvokerStorageHolder()?

No...unfortunately not without increasing the number of templates by 
sum(1..arity - 1).  There's a comment in prebind explaining this.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcod...
base/uber_callback.h:207: Callback(const internal::InvokerStorageHolder<T>&
invoker_holder)
On 2011/02/07 20:51:48, willchan wrote:
> Why is this const if we're calling swap on its member variable?

This is to avoid an Addref/Unref pair.   I'll add a comment.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcod...
base/uber_callback.h:342: typedef
R(*PolymorphicInvoke)(internal::InvokerStorageBase*, const A1&,
On 2011/02/06 10:26:50, willchan wrote:
> Nit: you should probably change the pump to emit a line for each of these
> arguments, so you don't have the strange formatting issues here.

Fixed...but I wonder if we're trading pump-file readability (which is already
pretty bad) for generated-code readability.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback.h#newcod...
base/uber_callback.h:351: R Run(const A1& a1, const A2& a2, const A3& a3, const
A4& a4, const A5& a5,
On 2011/02/06 10:26:50, willchan wrote:
> Ditto on the line per argument to avoid the formatting indentation issue.

Done.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback_helpers.h
File base/uber_callback_helpers.h (right):

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback_helpers....
base/uber_callback_helpers.h:33: }
On 2011/02/07 23:25:15, willchan wrote:
> Perhaps the destructor should DCHECK that invoker_storage_ is NULL, since
you're
> only ever using this to pass the concrete type to the function traits, during
> which you are taking ownership of the |invoker_storage_|.

We could...but then we'd be adding a non-trivial destructor.  Not sure if it's
worth it.

http://codereview.chromium.org/6109007/diff/11035/base/uber_callback_helpers....
base/uber_callback_helpers.h:34: mutable scoped_refptr<InvokerStorageBase>
invoker_storage_;
On 2011/02/06 10:26:50, willchan wrote:
> Why is this mutable?

Added comment explaining.

[willchan no longer on Chromium, 2011-02-08 22:24:29]

I talked to Albert about this. Last major comment is to shrink the public
headers to just the declarations. And defer the template specializations for
bind.h to an implementation header file. I want to make sure that callback.h and
bind.h are short and sweet so users don't have to sift through the
implementation details to figure out how to use it. Understanding the
implementation details are for sadomasochists with too much spare time.

Also, please make sure that it's obvious from the comments how the lifetime is
managed (that it's using a refcounted internal class) so users don't need to
wonder if they need to delete anything.

I didn't review the unit tests, I hope someone else has looked at it :P

All in all, it lgtm though. I have no other major issues.

Albert, you and I talked offline about some of the reasons the implementation
details are the way they are. It'd be great if the implementation comments
reflected the decisions, such as the implicit constructor and mutable member
variable.

[awong, 2011-02-09 00:02:08]

Thanks Wiil for wading through all of that with me.

I did two of the renames, and  split-out the internal namespace from bind.h  to
bind_internal.h.

Darin, Brett, I think the reviews in your court now.

Regarding Unretained versus ConstRef, I've been thinking about it more and more,
and am really unconvinced that we'd be gaining much by mangling more information
into the name.

Behaviorally, I see unclarity in 2 scenarios:
  1) All Chrome-devs during the conversion.
  2) New hires + contributors.

For #1, this is a one-time hit and I think we can mitigate with documentation +
techtalk.

For #2, I believe this is invalid in this case because, this will become similar
to current constructs like "MessageLoop", which no matter how you name it, just
requires someone to go stare at it a bit before understanding.

The tradeoff of longer names is that it actually will hurt readability at point
of usage.

Making the names longer feels like it won't really solve the fundamental issues,
and will hurt readability in usage.

Thoughts?

[awong, 2011-02-09 00:02:10]

Thanks Wiil for wading through all of that with me.

I did two of the renames, and  split-out the internal namespace from bind.h  to
bind_internal.h.

Darin, Brett, I think the reviews in your court now.

Regarding Unretained versus ConstRef, I've been thinking about it more and more,
and am really unconvinced that we'd be gaining much by mangling more information
into the name.

Behaviorally, I see unclarity in 2 scenarios:
  1) All Chrome-devs during the conversion.
  2) New hires + contributors.

For #1, this is a one-time hit and I think we can mitigate with documentation +
techtalk.

For #2, I believe this is invalid in this case because, this will become similar
to current constructs like "MessageLoop", which no matter how you name it, just
requires someone to go stare at it a bit before understanding.

The tradeoff of longer names is that it actually will hurt readability at point
of usage.

Making the names longer feels like it won't really solve the fundamental issues,
and will hurt readability in usage.

Thoughts?

[willchan no longer on Chromium, 2011-02-09 01:15:32]

On Tue, Feb 8, 2011 at 4:02 PM, <ajwong@chromium.org> wrote:

> Thanks Wiil for wading through all of that with me.
>
> I did two of the renames, and  split-out the internal namespace from bind.h
>  to
> bind_internal.h.
>
> Darin, Brett, I think the reviews in your court now.
>
> Regarding Unretained versus ConstRef, I've been thinking about it more and
> more,
> and am really unconvinced that we'd be gaining much by mangling more
> information
> into the name.
>

FWIW, I'm with Albert on this one. I don't feel strongly though. So feel
free to ignore my opinion here.

PS: I don't think Albert meant versus here. To be clear, I am happy with the
current names Unretained and ConstRef.


>
> Behaviorally, I see unclarity in 2 scenarios:
>  1) All Chrome-devs during the conversion.
>  2) New hires + contributors.
>
> For #1, this is a one-time hit and I think we can mitigate with
> documentation +
> techtalk.
>
> For #2, I believe this is invalid in this case because, this will become
> similar
> to current constructs like "MessageLoop", which no matter how you name it,
> just
> requires someone to go stare at it a bit before understanding.
>
> The tradeoff of longer names is that it actually will hurt readability at
> point
> of usage.
>
> Making the names longer feels like it won't really solve the fundamental
> issues,
> and will hurt readability in usage.
>
> Thoughts?
>
>
> http://codereview.chromium.org/6109007/
>

[brettw, 2011-02-10 17:41:24]

I looked over this briefly (finally). It looks pretty nice and I'm going to try
to avoid bikeshedding on names.

http://codereview.chromium.org/6109007/diff/46016/base/callback.h
File base/callback.h (right):

http://codereview.chromium.org/6109007/diff/46016/base/callback.h#newcode142
base/callback.h:142: // wish to pass the invoked object a reference to the bound
parameter.
These two paragraphs about ConstRef and Unretained don't really illuminate much
for me. I'd probably remove them and reference the documentation in
bind_helpers.h after the sentence above "...and wrap a pointer to argument."

[darin (slow to review), 2011-02-10 19:09:14]

OK, I'm happy to stick with Unretained and ConstRef.  Thanks for making the
change from Prebind to Bind though!
-Darin

On Tue, Feb 8, 2011 at 4:02 PM, <ajwong@chromium.org> wrote:

> Thanks Wiil for wading through all of that with me.
>
> I did two of the renames, and  split-out the internal namespace from bind.h
>  to
> bind_internal.h.
>
> Darin, Brett, I think the reviews in your court now.
>
> Regarding Unretained versus ConstRef, I've been thinking about it more and
> more,
> and am really unconvinced that we'd be gaining much by mangling more
> information
> into the name.
>
> Behaviorally, I see unclarity in 2 scenarios:
>  1) All Chrome-devs during the conversion.
>  2) New hires + contributors.
>
> For #1, this is a one-time hit and I think we can mitigate with
> documentation +
> techtalk.
>
> For #2, I believe this is invalid in this case because, this will become
> similar
> to current constructs like "MessageLoop", which no matter how you name it,
> just
> requires someone to go stare at it a bit before understanding.
>
> The tradeoff of longer names is that it actually will hurt readability at
> point
> of usage.
>
> Making the names longer feels like it won't really solve the fundamental
> issues,
> and will hurt readability in usage.
>
> Thoughts?
>
>
> http://codereview.chromium.org/6109007/
>

[darin (slow to review), 2011-02-12 00:36:07]

looks great overall.  i defer final LGTM to will.... here's a few thoughts:

http://codereview.chromium.org/6109007/diff/46016/base/bind_helpers.h
File base/bind_helpers.h (right):

http://codereview.chromium.org/6109007/diff/46016/base/bind_helpers.h#newcode124
base/bind_helpers.h:124: class SupportsAddRefAndRelease {
note: might be nice to move this to ref_counted.h

http://codereview.chromium.org/6109007/diff/46016/base/bind_helpers.h#newcode196
base/bind_helpers.h:196: template <typename ref, typename O>
nit: "O" is not the best template parameter.

http://codereview.chromium.org/6109007/diff/46016/base/bind_helpers.h#newcode270
base/bind_helpers.h:270:
COMPILE_ASSERT(internal::SupportsAddRefAndRelease<T>::value == false,
even though i might have argued for this in the past, i suspect that it may be
overly restrictive.  i'm thinking of cases related to object destruction time,
when you might want to pass a reference counted object to a function whose job
it is to delete the object (i.e., DeleteSoon).

[awong, 2011-02-12 09:44:45]

Addressed all comments.

Will, would you be willing to do the honors of the final LGTM?  The last set of
changes are all cosmetic except for removing the COMPILE_ASSERT from Unretained
(see Darin's comment).

Brett, Darin -- thanks for being flexible on the naming.  I think this will end
up being the right path.

http://codereview.chromium.org/6109007/diff/46016/base/bind_helpers.h
File base/bind_helpers.h (right):

http://codereview.chromium.org/6109007/diff/46016/base/bind_helpers.h#newcode124
base/bind_helpers.h:124: class SupportsAddRefAndRelease {
On 2011/02/12 00:36:07, darin wrote:
> note: might be nice to move this to ref_counted.h

Added TODO to move it later.

http://codereview.chromium.org/6109007/diff/46016/base/bind_helpers.h#newcode196
base/bind_helpers.h:196: template <typename ref, typename O>
On 2011/02/12 00:36:07, darin wrote:
> nit: "O" is not the best template parameter.

Done.

http://codereview.chromium.org/6109007/diff/46016/base/bind_helpers.h#newcode270
base/bind_helpers.h:270:
COMPILE_ASSERT(internal::SupportsAddRefAndRelease<T>::value == false,
On 2011/02/12 00:36:07, darin wrote:
> even though i might have argued for this in the past, i suspect that it may be
> overly restrictive.  i'm thinking of cases related to object destruction time,
> when you might want to pass a reference counted object to a function whose job
> it is to delete the object (i.e., DeleteSoon).

Removed.

http://codereview.chromium.org/6109007/diff/46016/base/callback.h
File base/callback.h (right):

http://codereview.chromium.org/6109007/diff/46016/base/callback.h#newcode142
base/callback.h:142: // wish to pass the invoked object a reference to the bound
parameter.
On 2011/02/10 17:41:24, brettw wrote:
> These two paragraphs about ConstRef and Unretained don't really illuminate
much
> for me. I'd probably remove them and reference the documentation in
> bind_helpers.h after the sentence above "...and wrap a pointer to argument."

Done.

[akalin, 2011-02-12 21:33:21]

Looks pretty solid!

http://codereview.chromium.org/6109007/diff/54001/base/bind_unittest.cc
File base/bind_unittest.cc (right):

http://codereview.chromium.org/6109007/diff/54001/base/bind_unittest.cc#newco...
base/bind_unittest.cc:232: Callback<int(int)> c1= Bind(&Sum, 32, 16, 8, 4, 2);
whitespace before = (here and the lines below)

[awong, 2011-02-13 03:55:01]

Fixed Fred's comments.

Also found a bug in the argument type checking for the AddRef/Release check
while porting MessageLoop unittest.  Had to add an is_class trait (<grumble />
<grumble />).

http://codereview.chromium.org/6109007/diff/54001/base/bind_unittest.cc
File base/bind_unittest.cc (right):

http://codereview.chromium.org/6109007/diff/54001/base/bind_unittest.cc#newco...
base/bind_unittest.cc:232: Callback<int(int)> c1= Bind(&Sum, 32, 16, 8, 4, 2);
On 2011/02/12 21:33:22, akalin wrote:
> whitespace before = (here and the lines below)

Done.

[awong, 2011-02-13 03:58:46]

Crap...I broke windows.  Will fix monday.  Grr...

[awong, 2011-02-13 07:10:26]

Apparently today is "monday".  Fixed!

Ready for another look.

On 2011/02/13 03:58:46, awong wrote:
> Crap...I broke windows.  Will fix monday.  Grr...

[willchan no longer on Chromium, 2011-02-15 01:17:14]

LGTM! Ship it.

http://codereview.chromium.org/6109007/diff/58002/base/bind_helpers.h
File base/bind_helpers.h (right):

http://codereview.chromium.org/6109007/diff/58002/base/bind_helpers.h#newcode71
base/bind_helpers.h:71: // For SFINAE to work with inherited methods, we need to
pull some extra ticks
ticks=>tricks

http://codereview.chromium.org/6109007/diff/58002/base/bind_unittest.cc
File base/bind_unittest.cc (right):

http://codereview.chromium.org/6109007/diff/58002/base/bind_unittest.cc#newco...
base/bind_unittest.cc:10: #error "base/bind.h should avoid pulling in callack.h
by default."
callback.h

http://codereview.chromium.org/6109007/diff/58002/base/bind_unittest.cc#newco...
base/bind_unittest.cc:269: &has_ref_);
Indentation is off here, probably from the rename from Prebind to Bind.

http://codereview.chromium.org/6109007/diff/58002/base/bind_unittest.cc#newco...
base/bind_unittest.cc:271: const_has_ref_ptr_);
Indentation is off here, probably from the rename from Prebind to Bind.

http://codereview.chromium.org/6109007/diff/58002/base/template_util_unittest.cc
File base/template_util_unittest.cc (right):

http://codereview.chromium.org/6109007/diff/58002/base/template_util_unittest...
base/template_util_unittest.cc:41: // TODO(ajwong): Why is is_convertible
disabled on windows?
I couldn't ever get it to compile in Windows :P You seem to be better (or at
least more diligent!) about getting this to work on all compilers, so maybe you
could fix this too :P

http://codereview.chromium.org/6109007/diff/58002/base/template_util_unittest...
base/template_util_unittest.cc:42: TEST(TemplateUtilTest, IsConvertable) {
IsConvertable=>IsConvertible