Callback API Change: is_null, Reset, and Equals

Emptiness, Reset, and Comparison API for Callbacks.

Since Callback<> is essentially a smartpointer, some introspective APIs are
required for sensible usage.

BUG=35223
TEST=new unittests

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

[awong, 2011-02-17 00:53:34]

I chose the following 3 function names.  I'm not attached to them, and am
amenable to some (small)amount of bike-shedding on them. :)

-Albert

[darin (slow to review), 2011-02-17 00:59:24]

As far as naming goes, I only wonder if "is_null" would be better than
"is_empty"...

[willchan no longer on Chromium, 2011-02-17 01:09:57]

I'm not going to review the unittest. I'm punting to akalin for that. Thanks
Fred :)

http://codereview.chromium.org/6507029/diff/3005/base/callback.h
File base/callback.h (right):

http://codereview.chromium.org/6507029/diff/3005/base/callback.h#newcode227
base/callback.h:227: explicit CallbackBase(InvokeFuncStorage polymorphic_invoke)
Can't this be protected? And then you can move all these accessors to public,
above the initial protected: section? Then you won't need to have them out of
order and won't need 2 protected sections.

http://codereview.chromium.org/6507029/diff/3005/base/callback.h#newcode232
base/callback.h:232: bool is_empty() const {
Bike shed alert: I prefer empty() to is_empty(). For some reason, the latter
sounds fugly to me. This is just me being neurotic, so feel free to ignore.

To add a minor bit of weight behind my bike shed comment, I refer to the
tr1::function proposal document
(http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2002/n1402.html) which says:

=====

IIIc. Lack of extraneous null-checking syntax

This follows from the lack of comparison operators. For a function object
wrapper f, there is no syntax f != 0, f == 0, etc. The allowed syntactic
constructs for checking for an empty (null) function object wrapper are f or !f
in a boolean context, (bool)f or f.empty().

=====

So I favor empty(). And if it becomes useful, then maybe we can add the other
operators but I figure let's not bother unless people demand it.

[akalin, 2011-02-17 01:16:39]

http://codereview.chromium.org/6507029/diff/3005/base/callback.h
File base/callback.h (left):

http://codereview.chromium.org/6507029/diff/3005/base/callback.h#oldcode242
base/callback.h:242: invoker_storage_.swap(invoker_holder.invoker_storage_);
can't you add an InvokerStorageBase param to the CallbackBase constructor and do
the (and const cast) swap there?

http://codereview.chromium.org/6507029/diff/3005/base/callback.h
File base/callback.h (right):

http://codereview.chromium.org/6507029/diff/3005/base/callback.h#newcode242
base/callback.h:242: bool Equals(const CallbackBase& other) const {
Do we really need this?  I have a hard time imagining when I'd need to compare
callbacks, although I guess it may help when stuffing them in a container.

Bikeshed alert: Why is Equals() uppercase but all the other methods lowercase?

[awong, 2011-02-17 02:05:29]

Went with empty() right now, and lower-cased equals.  They were difference
conventions cause "reset/is_empty" was psuedo-following the C++ style where
"Equals" was the google style.

As for the emptiness check, my preference is

  empty() > is_null() >>> is_empty().

I could go either way between empty() and is_null().

http://codereview.chromium.org/6507029/diff/3005/base/callback.h
File base/callback.h (left):

http://codereview.chromium.org/6507029/diff/3005/base/callback.h#oldcode242
base/callback.h:242: invoker_storage_.swap(invoker_holder.invoker_storage_);
On 2011/02/17 01:16:39, akalin wrote:
> can't you add an InvokerStorageBase param to the CallbackBase constructor and
do
> the (and const cast) swap there?

Oh...good point.

http://codereview.chromium.org/6507029/diff/3005/base/callback.h
File base/callback.h (right):

http://codereview.chromium.org/6507029/diff/3005/base/callback.h#newcode227
base/callback.h:227: explicit CallbackBase(InvokeFuncStorage polymorphic_invoke)
On 2011/02/17 01:09:57, willchan wrote:
> Can't this be protected? And then you can move all these accessors to public,
> above the initial protected: section? Then you won't need to have them out of
> order and won't need 2 protected sections.

Good point. Fixed.

http://codereview.chromium.org/6507029/diff/3005/base/callback.h#newcode232
base/callback.h:232: bool is_empty() const {
On 2011/02/17 01:09:57, willchan wrote:
> Bike shed alert: I prefer empty() to is_empty(). For some reason, the latter
> sounds fugly to me. This is just me being neurotic, so feel free to ignore.
> 
> To add a minor bit of weight behind my bike shed comment, I refer to the
> tr1::function proposal document
> (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2002/n1402.html) which
says:
> 
> =====
> 
> IIIc. Lack of extraneous null-checking syntax
> 
> This follows from the lack of comparison operators. For a function object
> wrapper f, there is no syntax f != 0, f == 0, etc. The allowed syntactic
> constructs for checking for an empty (null) function object wrapper are f or
!f
> in a boolean context, (bool)f or f.empty().
> 
> =====
> 
> So I favor empty(). And if it becomes useful, then maybe we can add the other
> operators but I figure let's not bother unless people demand it.


Yeah...now that you mention it, empty() is my preference as well.

http://codereview.chromium.org/6507029/diff/3005/base/callback.h#newcode242
base/callback.h:242: bool Equals(const CallbackBase& other) const {
On 2011/02/17 01:16:39, akalin wrote:
> Do we really need this?  I have a hard time imagining when I'd need to compare
> callbacks, although I guess it may help when stuffing them in a container.
> 
> Bikeshed alert: Why is Equals() uppercase but all the other methods lowercase?

I struggled with this...I almost actually wanted to overload operator ==.

The usecase is actually in messageloop.  There is a point where they currently
test "Task*" for equality.  I can drop this for now and try to find a way in
message-loop to tag a change differently.  That might be cleaner.

[willchan no longer on Chromium, 2011-02-17 02:20:47]

On 2011/02/17 02:05:29, awong wrote:
> Went with empty() right now, and lower-cased equals.  They were difference
> conventions cause "reset/is_empty" was psuedo-following the C++ style where
> "Equals" was the google style.

<bikeshed>
I guess I'd push for the Google style here. Reset() and Equals(). I say this
because the subclasses all do Run(), which already isn't C++ style. So I think
that'd be more consistent. I am more ambivalent on empty() because it's more
like an accessor of sorts, but am open to capitalizing it too.
</bikeshed>
> 
> As for the emptiness check, my preference is
> 
>   empty() > is_null() >>> is_empty().
> 
> I could go either way between empty() and is_null().
> 
> http://codereview.chromium.org/6507029/diff/3005/base/callback.h
> File base/callback.h (left):
> 
> http://codereview.chromium.org/6507029/diff/3005/base/callback.h#oldcode242
> base/callback.h:242: invoker_storage_.swap(invoker_holder.invoker_storage_);
> On 2011/02/17 01:16:39, akalin wrote:
> > can't you add an InvokerStorageBase param to the CallbackBase constructor
and
> do
> > the (and const cast) swap there?
> 
> Oh...good point.
> 
> http://codereview.chromium.org/6507029/diff/3005/base/callback.h
> File base/callback.h (right):
> 
> http://codereview.chromium.org/6507029/diff/3005/base/callback.h#newcode227
> base/callback.h:227: explicit CallbackBase(InvokeFuncStorage
polymorphic_invoke)
> On 2011/02/17 01:09:57, willchan wrote:
> > Can't this be protected? And then you can move all these accessors to
public,
> > above the initial protected: section? Then you won't need to have them out
of
> > order and won't need 2 protected sections.
> 
> Good point. Fixed.
> 
> http://codereview.chromium.org/6507029/diff/3005/base/callback.h#newcode232
> base/callback.h:232: bool is_empty() const {
> On 2011/02/17 01:09:57, willchan wrote:
> > Bike shed alert: I prefer empty() to is_empty(). For some reason, the latter
> > sounds fugly to me. This is just me being neurotic, so feel free to ignore.
> > 
> > To add a minor bit of weight behind my bike shed comment, I refer to the
> > tr1::function proposal document
> > (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2002/n1402.html) which
> says:
> > 
> > =====
> > 
> > IIIc. Lack of extraneous null-checking syntax
> > 
> > This follows from the lack of comparison operators. For a function object
> > wrapper f, there is no syntax f != 0, f == 0, etc. The allowed syntactic
> > constructs for checking for an empty (null) function object wrapper are f or
> !f
> > in a boolean context, (bool)f or f.empty().
> > 
> > =====
> > 
> > So I favor empty(). And if it becomes useful, then maybe we can add the
other
> > operators but I figure let's not bother unless people demand it.
> 
> 
> Yeah...now that you mention it, empty() is my preference as well.
> 
> http://codereview.chromium.org/6507029/diff/3005/base/callback.h#newcode242
> base/callback.h:242: bool Equals(const CallbackBase& other) const {
> On 2011/02/17 01:16:39, akalin wrote:
> > Do we really need this?  I have a hard time imagining when I'd need to
compare
> > callbacks, although I guess it may help when stuffing them in a container.
> > 
> > Bikeshed alert: Why is Equals() uppercase but all the other methods
lowercase?
> 
> I struggled with this...I almost actually wanted to overload operator ==.
> 
> The usecase is actually in messageloop.  There is a point where they currently
> test "Task*" for equality.  I can drop this for now and try to find a way in
> message-loop to tag a change differently.  That might be cleaner.

[darin (slow to review), 2011-02-17 06:46:35]

On Wed, Feb 16, 2011 at 6:20 PM, <willchan@chromium.org> wrote:

> On 2011/02/17 02:05:29, awong wrote:
>
>> Went with empty() right now, and lower-cased equals.  They were difference
>> conventions cause "reset/is_empty" was psuedo-following the C++ style
>> where
>> "Equals" was the google style.
>>
>
> <bikeshed>
> I guess I'd push for the Google style here. Reset() and Equals(). I say
> this
> because the subclasses all do Run(), which already isn't C++ style. So I
> think
> that'd be more consistent. I am more ambivalent on empty() because it's
> more
> like an accessor of sorts, but am open to capitalizing it too.
> </bikeshed>


/nod

We should default to what Google C++ style rules would suggest unless we
have
a very good reason to do otherwise.

Also, we have convention in Chromium to put the "is_" prefix in front of
"empty",
so that it reads like a question.

-Darin




>
>
>  As for the emptiness check, my preference is
>>
>
>    empty() > is_null() >>> is_empty().
>>
>
>  I could go either way between empty() and is_null().
>>
>
>  http://codereview.chromium.org/6507029/diff/3005/base/callback.h
>> File base/callback.h (left):
>>
>
>
>> http://codereview.chromium.org/6507029/diff/3005/base/callback.h#oldcode242
>> base/callback.h:242:
>> invoker_storage_.swap(invoker_holder.invoker_storage_);
>> On 2011/02/17 01:16:39, akalin wrote:
>> > can't you add an InvokerStorageBase param to the CallbackBase
>> constructor
>>
> and
>
>> do
>> > the (and const cast) swap there?
>>
>
>  Oh...good point.
>>
>
>  http://codereview.chromium.org/6507029/diff/3005/base/callback.h
>> File base/callback.h (right):
>>
>
>
>> http://codereview.chromium.org/6507029/diff/3005/base/callback.h#newcode227
>> base/callback.h:227: explicit CallbackBase(InvokeFuncStorage
>>
> polymorphic_invoke)
>
>> On 2011/02/17 01:09:57, willchan wrote:
>> > Can't this be protected? And then you can move all these accessors to
>>
> public,
>
>> > above the initial protected: section? Then you won't need to have them
>> out
>>
> of
>
>> > order and won't need 2 protected sections.
>>
>
>  Good point. Fixed.
>>
>
>
>> http://codereview.chromium.org/6507029/diff/3005/base/callback.h#newcode232
>> base/callback.h:232: bool is_empty() const {
>> On 2011/02/17 01:09:57, willchan wrote:
>> > Bike shed alert: I prefer empty() to is_empty(). For some reason, the
>> latter
>> > sounds fugly to me. This is just me being neurotic, so feel free to
>> ignore.
>> >
>> > To add a minor bit of weight behind my bike shed comment, I refer to the
>> > tr1::function proposal document
>> > (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2002/n1402.html)
>> which
>> says:
>> >
>> > =====
>> >
>> > IIIc. Lack of extraneous null-checking syntax
>> >
>> > This follows from the lack of comparison operators. For a function
>> object
>> > wrapper f, there is no syntax f != 0, f == 0, etc. The allowed syntactic
>> > constructs for checking for an empty (null) function object wrapper are
>> f or
>> !f
>> > in a boolean context, (bool)f or f.empty().
>> >
>> > =====
>> >
>> > So I favor empty(). And if it becomes useful, then maybe we can add the
>>
> other
>
>> > operators but I figure let's not bother unless people demand it.
>>
>
>
>  Yeah...now that you mention it, empty() is my preference as well.
>>
>
>
>> http://codereview.chromium.org/6507029/diff/3005/base/callback.h#newcode242
>> base/callback.h:242: bool Equals(const CallbackBase& other) const {
>> On 2011/02/17 01:16:39, akalin wrote:
>> > Do we really need this?  I have a hard time imagining when I'd need to
>>
> compare
>
>> > callbacks, although I guess it may help when stuffing them in a
>> container.
>> >
>> > Bikeshed alert: Why is Equals() uppercase but all the other methods
>>
> lowercase?
>
>  I struggled with this...I almost actually wanted to overload operator ==.
>>
>
>  The usecase is actually in messageloop.  There is a point where they
>> currently
>> test "Task*" for equality.  I can drop this for now and try to find a way
>> in
>> message-loop to tag a change differently.  That might be cleaner.
>>
>
>
>
> http://codereview.chromium.org/6507029/
>

[awong, 2011-02-17 18:32:54]

Current version:
  Equals()
  Reset()
  is_null()

Why is_null?  I did a coarse search for IsNull() and is_null() over both
chromium and internal google source in header files.  is_null() seems slightly
more prevalent.

In particular, for Chromium, is_null() is in ppapi, time.h, and
nullable_string.h.  IsNull seems mostly to be in WebKit, V8, WTL, and other
third-partyish things.

So, going with is_null() right now.

[awong, 2011-02-17 18:33:37]

is_null() code_search:
 
http://www.google.com/codesearch?hl=en&vert=chromium&lr=&q=file:.\.h+is_null\...

IsNull() code search:
 
http://www.google.com/codesearch?hl=en&vert=chromium&lr=&q=file:.\.h+IsNull\(...


On 2011/02/17 18:32:54, awong wrote:
> Current version:
>   Equals()
>   Reset()
>   is_null()
> 
> Why is_null?  I did a coarse search for IsNull() and is_null() over both
> chromium and internal google source in header files.  is_null() seems slightly
> more prevalent.
> 
> In particular, for Chromium, is_null() is in ppapi, time.h, and
> nullable_string.h.  IsNull seems mostly to be in WebKit, V8, WTL, and other
> third-partyish things.
> 
> So, going with is_null() right now.

[darin (slow to review), 2011-02-17 18:40:31]

sgtm

On Thu, Feb 17, 2011 at 10:32 AM, <ajwong@chromium.org> wrote:

> Current version:
>  Equals()
>  Reset()
>  is_null()
>
> Why is_null?  I did a coarse search for IsNull() and is_null() over both
> chromium and internal google source in header files.  is_null() seems
> slightly
> more prevalent.
>
> In particular, for Chromium, is_null() is in ppapi, time.h, and
> nullable_string.h.  IsNull seems mostly to be in WebKit, V8, WTL, and other
> third-partyish things.
>
> So, going with is_null() right now.
>
>
>
> http://codereview.chromium.org/6507029/
>

[akalin, 2011-02-17 20:29:44]

LGTM (I think we all agreed on the function names?)

http://codereview.chromium.org/6507029/diff/6002/base/callback_unittest.cc
File base/callback_unittest.cc (right):

http://codereview.chromium.org/6507029/diff/6002/base/callback_unittest.cc#ne...
base/callback_unittest.cc:120: ASSERT_FALSE(callback_a_.is_null());
no need for these to be asserts, I think

[willchan no longer on Chromium, 2011-02-17 20:40:21]

I'm fine with the names.

On Thu, Feb 17, 2011 at 12:29 PM, <akalin@chromium.org> wrote:

> LGTM (I think we all agreed on the function names?)
>
>
> http://codereview.chromium.org/6507029/diff/6002/base/callback_unittest.cc
> File base/callback_unittest.cc (right):
>
>
>
http://codereview.chromium.org/6507029/diff/6002/base/callback_unittest.cc#ne...
> base/callback_unittest.cc:120: ASSERT_FALSE(callback_a_.is_null());
> no need for these to be asserts, I think
>
>
> http://codereview.chromium.org/6507029/
>

[awong, 2011-02-18 00:42:41]

http://codereview.chromium.org/6507029/diff/6002/base/callback_unittest.cc
File base/callback_unittest.cc (right):

http://codereview.chromium.org/6507029/diff/6002/base/callback_unittest.cc#ne...
base/callback_unittest.cc:120: ASSERT_FALSE(callback_a_.is_null());
On 2011/02/17 20:29:44, akalin wrote:
> no need for these to be asserts, I think

The rest of the test is invalid if these assumptions are false...

[akalin, 2011-02-18 00:59:20]

On 2011/02/18 00:42:41, awong wrote:
> http://codereview.chromium.org/6507029/diff/6002/base/callback_unittest.cc
> File base/callback_unittest.cc (right):
> 
>
http://codereview.chromium.org/6507029/diff/6002/base/callback_unittest.cc#ne...
> base/callback_unittest.cc:120: ASSERT_FALSE(callback_a_.is_null());
> On 2011/02/17 20:29:44, akalin wrote:
> > no need for these to be asserts, I think
> 
> The rest of the test is invalid if these assumptions are false...

That's true, but usually ASSERT_ is used only to avoid crashes, i.e. ASSERT(ptr
!= NULL), etc.  But I don't care too much, I'm fine with leaving it as is.

[awong, 2011-02-18 01:46:08]

On 2011/02/18 00:59:20, akalin wrote:
> On 2011/02/18 00:42:41, awong wrote:
> > http://codereview.chromium.org/6507029/diff/6002/base/callback_unittest.cc
> > File base/callback_unittest.cc (right):
> > 
> >
>
http://codereview.chromium.org/6507029/diff/6002/base/callback_unittest.cc#ne...
> > base/callback_unittest.cc:120: ASSERT_FALSE(callback_a_.is_null());
> > On 2011/02/17 20:29:44, akalin wrote:
> > > no need for these to be asserts, I think
> > 
> > The rest of the test is invalid if these assumptions are false...
> 
> That's true, but usually ASSERT_ is used only to avoid crashes, i.e.
ASSERT(ptr
> != NULL), etc.  But I don't care too much, I'm fine with leaving it as is.

<digression>
I disagree with this slightly.  Even in the absence of crashes, ASSERT_XX is
less-favorable because it stops a test early when other cases may still be able
to provide useful info. If the cannot produce sensible result after a failure,
then using an EXPECT_XX just muddies the failing test run with unpredictable (or
noisy) results.
</digression>

[awong, 2011-02-18 01:46:16]

(checking in).