Callback API Change: Add in an Owned() wrapper to base::Bind().

Add in an Owned() wrapper to base::Bind().

This allows expression of ownership of a pointer by a callback. It's basically a "scoped_ptr<>" for Callback where the scope tied to the callback object.

BUG=96118
TEST=new unittests.

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

[awong, 2011-10-08 19:10:57]

Here's a proposal for Owned().

Please look at the unittest mainly and tell me what you think of the usage. 
There are 2 issues I'd particularly like feedback on.

  1) What to name these things?  MaybeCapturePtr sounds awkward.
  2) Do you like the ownership transfer semantics of forcing a type like
MaybeCapturePtr.

Also note that htis only works with things that are "new"ed.  Doesn't work with
new[], or malloc.

[awong, 2011-10-08 20:48:07]

The thing I'm struggling the most with this design is that MaybeCapturePtr feels
a lot like PassOwnPtr, but just different enough.

One could argue that the interface should just work like:

void Func(PassOwnPtr<Foo> f) {
  if (f) {
    delete f.release();
    cout << "released!\n";
  } else {
    cout << "was null.\n";
  }
}

Closure cb = Bind(&Func(PassOwnPtr<new Foo()>));

cb.Run();  // Prints "released!"
cb.Run();  // Prints "was null."

This has less types in the system, but would require PassOwnPtr to be much
smarter than what's in WebKit today (it'd need to have similar functionality to
MaybeCapturesPtr in my implementation).  Also, the implementor of Foo has less
indication that the PassOwnPtr might come from a callback, and thus they might
need to consider nullness of the argument.

Thoughts?

-Albert


On 2011/10/08 19:10:57, awong wrote:
> Here's a proposal for Owned().
> 
> Please look at the unittest mainly and tell me what you think of the usage. 
> There are 2 issues I'd particularly like feedback on.
> 
>   1) What to name these things?  MaybeCapturePtr sounds awkward.
>   2) Do you like the ownership transfer semantics of forcing a type like
> MaybeCapturePtr.
> 
> Also note that htis only works with things that are "new"ed.  Doesn't work
with
> new[], or malloc.

[awong, 2011-10-09 06:36:14]

One last possibility, we could just make this a simple Owned() concept, and only
pass back raw pointers without anything like MaybeCaptures, or PassOwnPtr. 
Then, if someone wants to "pass ownership" they would do

void Func(scoped_ptr<Foo>* f);

base::Bind(&Func, Owned(new scoped_ptr<Foo>(new Foo()));

That's pretty ugly too though.

On 2011/10/08 20:48:07, awong wrote:
> The thing I'm struggling the most with this design is that MaybeCapturePtr
feels
> a lot like PassOwnPtr, but just different enough.
> 
> One could argue that the interface should just work like:
> 
> void Func(PassOwnPtr<Foo> f) {
>   if (f) {
>     delete f.release();
>     cout << "released!\n";
>   } else {
>     cout << "was null.\n";
>   }
> }
> 
> Closure cb = Bind(&Func(PassOwnPtr<new Foo()>));
> 
> cb.Run();  // Prints "released!"
> cb.Run();  // Prints "was null."
> 
> This has less types in the system, but would require PassOwnPtr to be much
> smarter than what's in WebKit today (it'd need to have similar functionality
to
> MaybeCapturesPtr in my implementation).  Also, the implementor of Foo has less
> indication that the PassOwnPtr might come from a callback, and thus they might
> need to consider nullness of the argument.
> 
> Thoughts?
> 
> -Albert
> 
> 
> On 2011/10/08 19:10:57, awong wrote:
> > Here's a proposal for Owned().
> > 
> > Please look at the unittest mainly and tell me what you think of the usage. 
> > There are 2 issues I'd particularly like feedback on.
> > 
> >   1) What to name these things?  MaybeCapturePtr sounds awkward.
> >   2) Do you like the ownership transfer semantics of forcing a type like
> > MaybeCapturePtr.
> > 
> > Also note that htis only works with things that are "new"ed.  Doesn't work
> with
> > new[], or malloc.

[willchan no longer on Chromium, 2011-10-09 06:40:54]

Can we make it so base::Owned() creates a scoped_ptr<T> in the
InvokerStorage? And then, have DoInvoke unwrap all arguments, where
unwrapping a scoped_ptr<T> would call .release() on it?

On Sat, Oct 8, 2011 at 11:36 PM, <ajwong@chromium.org> wrote:

> One last possibility, we could just make this a simple Owned() concept, and
> only
> pass back raw pointers without anything like MaybeCaptures, or PassOwnPtr.
> Then, if someone wants to "pass ownership" they would do
>
> void Func(scoped_ptr<Foo>* f);
>
> base::Bind(&Func, Owned(new scoped_ptr<Foo>(new Foo()));
>
> That's pretty ugly too though.
>
>
> On 2011/10/08 20:48:07, awong wrote:
>
>> The thing I'm struggling the most with this design is that MaybeCapturePtr
>>
> feels
>
>> a lot like PassOwnPtr, but just different enough.
>>
>
>  One could argue that the interface should just work like:
>>
>
>  void Func(PassOwnPtr<Foo> f) {
>>   if (f) {
>>     delete f.release();
>>     cout << "released!\n";
>>   } else {
>>     cout << "was null.\n";
>>   }
>> }
>>
>
>  Closure cb = Bind(&Func(PassOwnPtr<new Foo()>));
>>
>
>  cb.Run();  // Prints "released!"
>> cb.Run();  // Prints "was null."
>>
>
>  This has less types in the system, but would require PassOwnPtr to be much
>> smarter than what's in WebKit today (it'd need to have similar
>> functionality
>>
> to
>
>> MaybeCapturesPtr in my implementation).  Also, the implementor of Foo has
>> less
>> indication that the PassOwnPtr might come from a callback, and thus they
>> might
>> need to consider nullness of the argument.
>>
>
>  Thoughts?
>>
>
>  -Albert
>>
>
>
>  On 2011/10/08 19:10:57, awong wrote:
>> > Here's a proposal for Owned().
>> >
>> > Please look at the unittest mainly and tell me what you think of the
>> usage.
>> > There are 2 issues I'd particularly like feedback on.
>> >
>> >   1) What to name these things?  MaybeCapturePtr sounds awkward.
>> >   2) Do you like the ownership transfer semantics of forcing a type like
>> > MaybeCapturePtr.
>> >
>> > Also note that htis only works with things that are "new"ed.  Doesn't
>> work
>> with
>> > new[], or malloc.
>>
>
>
>
>
http://codereview.chromium.**org/8209001/<http://codereview.chromium.org/8209...
>

[willchan no longer on Chromium, 2011-10-09 06:42:31]

Actually, sorry, not a scoped_ptr<T> because that's noncopyable. But a
base::internal type, like OwnPtr<T> or something.

On Sat, Oct 8, 2011 at 11:40 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> Can we make it so base::Owned() creates a scoped_ptr<T> in the
> InvokerStorage? And then, have DoInvoke unwrap all arguments, where
> unwrapping a scoped_ptr<T> would call .release() on it?
>
>
> On Sat, Oct 8, 2011 at 11:36 PM, <ajwong@chromium.org> wrote:
>
>> One last possibility, we could just make this a simple Owned() concept,
>> and only
>> pass back raw pointers without anything like MaybeCaptures, or PassOwnPtr.
>> Then, if someone wants to "pass ownership" they would do
>>
>> void Func(scoped_ptr<Foo>* f);
>>
>> base::Bind(&Func, Owned(new scoped_ptr<Foo>(new Foo()));
>>
>> That's pretty ugly too though.
>>
>>
>> On 2011/10/08 20:48:07, awong wrote:
>>
>>> The thing I'm struggling the most with this design is that
>>> MaybeCapturePtr
>>>
>> feels
>>
>>> a lot like PassOwnPtr, but just different enough.
>>>
>>
>>  One could argue that the interface should just work like:
>>>
>>
>>  void Func(PassOwnPtr<Foo> f) {
>>>   if (f) {
>>>     delete f.release();
>>>     cout << "released!\n";
>>>   } else {
>>>     cout << "was null.\n";
>>>   }
>>> }
>>>
>>
>>  Closure cb = Bind(&Func(PassOwnPtr<new Foo()>));
>>>
>>
>>  cb.Run();  // Prints "released!"
>>> cb.Run();  // Prints "was null."
>>>
>>
>>  This has less types in the system, but would require PassOwnPtr to be
>>> much
>>> smarter than what's in WebKit today (it'd need to have similar
>>> functionality
>>>
>> to
>>
>>> MaybeCapturesPtr in my implementation).  Also, the implementor of Foo has
>>> less
>>> indication that the PassOwnPtr might come from a callback, and thus they
>>> might
>>> need to consider nullness of the argument.
>>>
>>
>>  Thoughts?
>>>
>>
>>  -Albert
>>>
>>
>>
>>  On 2011/10/08 19:10:57, awong wrote:
>>> > Here's a proposal for Owned().
>>> >
>>> > Please look at the unittest mainly and tell me what you think of the
>>> usage.
>>> > There are 2 issues I'd particularly like feedback on.
>>> >
>>> >   1) What to name these things?  MaybeCapturePtr sounds awkward.
>>> >   2) Do you like the ownership transfer semantics of forcing a type
>>> like
>>> > MaybeCapturePtr.
>>> >
>>> > Also note that htis only works with things that are "new"ed.  Doesn't
>>> work
>>> with
>>> > new[], or malloc.
>>>
>>
>>
>>
>>
http://codereview.chromium.**org/8209001/<http://codereview.chromium.org/8209...
>>
>
>

[awong, 2011-10-09 06:47:43]

The main thing that I worry about is multiple runs of a callback.  For
instance, I think with a semantic where Owned() always released the pointer
to the bound function, you'd have the following semantics:

void Func(Foo* f) {
  // We need to remember that ownership for f is passed and delete
appropriately.
  f->DoSomething();
  delete f;
}

Closure cb = Bind(&Func, Owned(new Foo()));

cb.Run();  // Works fine.
cb.Run();  // |f| above would be NULL, and f->DoSomething() would crash.

-Albert


On Sat, Oct 8, 2011 at 11:40 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> Can we make it so base::Owned() creates a scoped_ptr<T> in the
> InvokerStorage? And then, have DoInvoke unwrap all arguments, where
> unwrapping a scoped_ptr<T> would call .release() on it?
>
>
> On Sat, Oct 8, 2011 at 11:36 PM, <ajwong@chromium.org> wrote:
>
>> One last possibility, we could just make this a simple Owned() concept,
>> and only
>> pass back raw pointers without anything like MaybeCaptures, or PassOwnPtr.
>> Then, if someone wants to "pass ownership" they would do
>>
>> void Func(scoped_ptr<Foo>* f);
>>
>> base::Bind(&Func, Owned(new scoped_ptr<Foo>(new Foo()));
>>
>> That's pretty ugly too though.
>>
>>
>> On 2011/10/08 20:48:07, awong wrote:
>>
>>> The thing I'm struggling the most with this design is that
>>> MaybeCapturePtr
>>>
>> feels
>>
>>> a lot like PassOwnPtr, but just different enough.
>>>
>>
>>  One could argue that the interface should just work like:
>>>
>>
>>  void Func(PassOwnPtr<Foo> f) {
>>>   if (f) {
>>>     delete f.release();
>>>     cout << "released!\n";
>>>   } else {
>>>     cout << "was null.\n";
>>>   }
>>> }
>>>
>>
>>  Closure cb = Bind(&Func(PassOwnPtr<new Foo()>));
>>>
>>
>>  cb.Run();  // Prints "released!"
>>> cb.Run();  // Prints "was null."
>>>
>>
>>  This has less types in the system, but would require PassOwnPtr to be
>>> much
>>> smarter than what's in WebKit today (it'd need to have similar
>>> functionality
>>>
>> to
>>
>>> MaybeCapturesPtr in my implementation).  Also, the implementor of Foo has
>>> less
>>> indication that the PassOwnPtr might come from a callback, and thus they
>>> might
>>> need to consider nullness of the argument.
>>>
>>
>>  Thoughts?
>>>
>>
>>  -Albert
>>>
>>
>>
>>  On 2011/10/08 19:10:57, awong wrote:
>>> > Here's a proposal for Owned().
>>> >
>>> > Please look at the unittest mainly and tell me what you think of the
>>> usage.
>>> > There are 2 issues I'd particularly like feedback on.
>>> >
>>> >   1) What to name these things?  MaybeCapturePtr sounds awkward.
>>> >   2) Do you like the ownership transfer semantics of forcing a type
>>> like
>>> > MaybeCapturePtr.
>>> >
>>> > Also note that htis only works with things that are "new"ed.  Doesn't
>>> work
>>> with
>>> > new[], or malloc.
>>>
>>
>>
>>
>>
http://codereview.chromium.**org/8209001/<http://codereview.chromium.org/8209...
>>
>
>

[awong, 2011-10-09 06:58:05]

On Sat, Oct 8, 2011 at 11:51 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> Isn't that true for all the proposals with base::Owned()? Anything that
> allows you to transfer ownership means it is unsafe to invoke the callback
> multiple times.


Not necessarily.  In the simplest setup, Owned just effectively does a
scoped_ptr<> inside InvokerStorage, and passes the raw pointer to the
function on each call.  There is no way for the called function to take
ownership of the pointer.  I like this a lot because InvokerStorage is still
immutable, and multiple invocations get the same argument values passed in.
 However, there critical shortcoming is that you cannot pass ownership.

My thought behind adding the MaybeCapturePtr<> type was that it would tell
the person writing the function that they had to make a conscious choice
about whether or not to try and take ownership of a pointer.  If they never
call arg.release(), then the callback is actually safe to run multiple
times.

-Albert



>
> It strikes me as something where we could solve it with assertions in the
> debug code without bloating the release code. Use assertions to enforce a
> callback that has a base::Owned parameter doesn't have Run() called multiple
> times.
>
>
> On Sat, Oct 8, 2011 at 11:47 PM, Albert J. Wong (王重傑) <ajwong@chromium.org
> > wrote:
>
>> The main thing that I worry about is multiple runs of a callback.  For
>> instance, I think with a semantic where Owned() always released the pointer
>> to the bound function, you'd have the following semantics:
>>
>> void Func(Foo* f) {
>>   // We need to remember that ownership for f is passed and delete
>> appropriately.
>>   f->DoSomething();
>>   delete f;
>> }
>>
>> Closure cb = Bind(&Func, Owned(new Foo()));
>>
>> cb.Run();  // Works fine.
>> cb.Run();  // |f| above would be NULL, and f->DoSomething() would crash.
>>
>> -Albert
>>
>>
>> On Sat, Oct 8, 2011 at 11:40 PM, William Chan (陈智昌) <
>> willchan@chromium.org> wrote:
>>
>>> Can we make it so base::Owned() creates a scoped_ptr<T> in the
>>> InvokerStorage? And then, have DoInvoke unwrap all arguments, where
>>> unwrapping a scoped_ptr<T> would call .release() on it?
>>>
>>>
>>> On Sat, Oct 8, 2011 at 11:36 PM, <ajwong@chromium.org> wrote:
>>>
>>>> One last possibility, we could just make this a simple Owned() concept,
>>>> and only
>>>> pass back raw pointers without anything like MaybeCaptures, or
>>>> PassOwnPtr.
>>>> Then, if someone wants to "pass ownership" they would do
>>>>
>>>> void Func(scoped_ptr<Foo>* f);
>>>>
>>>> base::Bind(&Func, Owned(new scoped_ptr<Foo>(new Foo()));
>>>>
>>>> That's pretty ugly too though.
>>>>
>>>>
>>>> On 2011/10/08 20:48:07, awong wrote:
>>>>
>>>>> The thing I'm struggling the most with this design is that
>>>>> MaybeCapturePtr
>>>>>
>>>> feels
>>>>
>>>>> a lot like PassOwnPtr, but just different enough.
>>>>>
>>>>
>>>>  One could argue that the interface should just work like:
>>>>>
>>>>
>>>>  void Func(PassOwnPtr<Foo> f) {
>>>>>   if (f) {
>>>>>     delete f.release();
>>>>>     cout << "released!\n";
>>>>>   } else {
>>>>>     cout << "was null.\n";
>>>>>   }
>>>>> }
>>>>>
>>>>
>>>>  Closure cb = Bind(&Func(PassOwnPtr<new Foo()>));
>>>>>
>>>>
>>>>  cb.Run();  // Prints "released!"
>>>>> cb.Run();  // Prints "was null."
>>>>>
>>>>
>>>>  This has less types in the system, but would require PassOwnPtr to be
>>>>> much
>>>>> smarter than what's in WebKit today (it'd need to have similar
>>>>> functionality
>>>>>
>>>> to
>>>>
>>>>> MaybeCapturesPtr in my implementation).  Also, the implementor of Foo
>>>>> has less
>>>>> indication that the PassOwnPtr might come from a callback, and thus
>>>>> they might
>>>>> need to consider nullness of the argument.
>>>>>
>>>>
>>>>  Thoughts?
>>>>>
>>>>
>>>>  -Albert
>>>>>
>>>>
>>>>
>>>>  On 2011/10/08 19:10:57, awong wrote:
>>>>> > Here's a proposal for Owned().
>>>>> >
>>>>> > Please look at the unittest mainly and tell me what you think of the
>>>>> usage.
>>>>> > There are 2 issues I'd particularly like feedback on.
>>>>> >
>>>>> >   1) What to name these things?  MaybeCapturePtr sounds awkward.
>>>>> >   2) Do you like the ownership transfer semantics of forcing a type
>>>>> like
>>>>> > MaybeCapturePtr.
>>>>> >
>>>>> > Also note that htis only works with things that are "new"ed.  Doesn't
>>>>> work
>>>>> with
>>>>> > new[], or malloc.
>>>>>
>>>>
>>>>
>>>>
>>>>
http://codereview.chromium.**org/8209001/<http://codereview.chromium.org/8209...
>>>>
>>>
>>>
>>
>

[willchan no longer on Chromium, 2011-10-09 07:16:50]

On Sat, Oct 8, 2011 at 11:57 PM, Albert J. Wong (王重傑)
<ajwong@chromium.org>wrote:

> On Sat, Oct 8, 2011 at 11:51 PM, William Chan (陈智昌) <willchan@chromium.org
> > wrote:
>
>> Isn't that true for all the proposals with base::Owned()? Anything that
>> allows you to transfer ownership means it is unsafe to invoke the callback
>> multiple times.
>
>
> Not necessarily.  In the simplest setup, Owned just effectively does a
> scoped_ptr<> inside InvokerStorage, and passes the raw pointer to the
> function on each call.  There is no way for the called function to take
> ownership of the pointer.  I like this a lot because InvokerStorage is still
> immutable, and multiple invocations get the same argument values passed in.
>  However, there critical shortcoming is that you cannot pass ownership.
>

Ah yes, this sounds nice. I like base::Owned() being used for this. How
about a separate class for the memory transfer case? Don't do any implicit
conversions in the class or base::Bind(), so the function prototype has to
have an instance of this class as a parameter. That'll help document that
the function is doing a memory transfer and the user needs to be careful.


> My thought behind adding the MaybeCapturePtr<> type was that it would tell
> the person writing the function that they had to make a conscious choice
> about whether or not to try and take ownership of a pointer.  If they never
> call arg.release(), then the callback is actually safe to run multiple
> times.
>

I think that's too confusing. I'd rather be explicit about the two different
cases here. One where the Callback takes complete ownership of the memory
and another where it's simply used for transferring ownership to the
function we invoke (but is useful to shut up valgrind leaks where the
Callback doesn't get run). I'd hope the latter case would be less common
than base::Owned(), so it'd draw more attention when used, and we could use
debug assertions to try to prevent multiple Run() calls.


>
> -Albert
>
>
>
>>
>> It strikes me as something where we could solve it with assertions in the
>> debug code without bloating the release code. Use assertions to enforce a
>> callback that has a base::Owned parameter doesn't have Run() called multiple
>> times.
>>
>>
>> On Sat, Oct 8, 2011 at 11:47 PM, Albert J. Wong (王重傑) <
>> ajwong@chromium.org> wrote:
>>
>>> The main thing that I worry about is multiple runs of a callback.  For
>>> instance, I think with a semantic where Owned() always released the pointer
>>> to the bound function, you'd have the following semantics:
>>>
>>> void Func(Foo* f) {
>>>   // We need to remember that ownership for f is passed and delete
>>> appropriately.
>>>   f->DoSomething();
>>>   delete f;
>>> }
>>>
>>> Closure cb = Bind(&Func, Owned(new Foo()));
>>>
>>> cb.Run();  // Works fine.
>>> cb.Run();  // |f| above would be NULL, and f->DoSomething() would crash.
>>>
>>> -Albert
>>>
>>>
>>> On Sat, Oct 8, 2011 at 11:40 PM, William Chan (陈智昌) <
>>> willchan@chromium.org> wrote:
>>>
>>>> Can we make it so base::Owned() creates a scoped_ptr<T> in the
>>>> InvokerStorage? And then, have DoInvoke unwrap all arguments, where
>>>> unwrapping a scoped_ptr<T> would call .release() on it?
>>>>
>>>>
>>>> On Sat, Oct 8, 2011 at 11:36 PM, <ajwong@chromium.org> wrote:
>>>>
>>>>> One last possibility, we could just make this a simple Owned() concept,
>>>>> and only
>>>>> pass back raw pointers without anything like MaybeCaptures, or
>>>>> PassOwnPtr.
>>>>> Then, if someone wants to "pass ownership" they would do
>>>>>
>>>>> void Func(scoped_ptr<Foo>* f);
>>>>>
>>>>> base::Bind(&Func, Owned(new scoped_ptr<Foo>(new Foo()));
>>>>>
>>>>> That's pretty ugly too though.
>>>>>
>>>>>
>>>>> On 2011/10/08 20:48:07, awong wrote:
>>>>>
>>>>>> The thing I'm struggling the most with this design is that
>>>>>> MaybeCapturePtr
>>>>>>
>>>>> feels
>>>>>
>>>>>> a lot like PassOwnPtr, but just different enough.
>>>>>>
>>>>>
>>>>>  One could argue that the interface should just work like:
>>>>>>
>>>>>
>>>>>  void Func(PassOwnPtr<Foo> f) {
>>>>>>   if (f) {
>>>>>>     delete f.release();
>>>>>>     cout << "released!\n";
>>>>>>   } else {
>>>>>>     cout << "was null.\n";
>>>>>>   }
>>>>>> }
>>>>>>
>>>>>
>>>>>  Closure cb = Bind(&Func(PassOwnPtr<new Foo()>));
>>>>>>
>>>>>
>>>>>  cb.Run();  // Prints "released!"
>>>>>> cb.Run();  // Prints "was null."
>>>>>>
>>>>>
>>>>>  This has less types in the system, but would require PassOwnPtr to be
>>>>>> much
>>>>>> smarter than what's in WebKit today (it'd need to have similar
>>>>>> functionality
>>>>>>
>>>>> to
>>>>>
>>>>>> MaybeCapturesPtr in my implementation).  Also, the implementor of Foo
>>>>>> has less
>>>>>> indication that the PassOwnPtr might come from a callback, and thus
>>>>>> they might
>>>>>> need to consider nullness of the argument.
>>>>>>
>>>>>
>>>>>  Thoughts?
>>>>>>
>>>>>
>>>>>  -Albert
>>>>>>
>>>>>
>>>>>
>>>>>  On 2011/10/08 19:10:57, awong wrote:
>>>>>> > Here's a proposal for Owned().
>>>>>> >
>>>>>> > Please look at the unittest mainly and tell me what you think of the
>>>>>> usage.
>>>>>> > There are 2 issues I'd particularly like feedback on.
>>>>>> >
>>>>>> >   1) What to name these things?  MaybeCapturePtr sounds awkward.
>>>>>> >   2) Do you like the ownership transfer semantics of forcing a type
>>>>>> like
>>>>>> > MaybeCapturePtr.
>>>>>> >
>>>>>> > Also note that htis only works with things that are "new"ed.
>>>>>>  Doesn't work
>>>>>> with
>>>>>> > new[], or malloc.
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
http://codereview.chromium.**org/8209001/<http://codereview.chromium.org/8209...
>>>>>
>>>>
>>>>
>>>
>>
>

[awong, 2011-10-09 07:23:42]

On Sun, Oct 9, 2011 at 12:16 AM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> On Sat, Oct 8, 2011 at 11:57 PM, Albert J. Wong (王重傑) <ajwong@chromium.org
> > wrote:
>
>> On Sat, Oct 8, 2011 at 11:51 PM, William Chan (陈智昌) <
>> willchan@chromium.org> wrote:
>>
>>> Isn't that true for all the proposals with base::Owned()? Anything that
>>> allows you to transfer ownership means it is unsafe to invoke the callback
>>> multiple times.
>>
>>
>> Not necessarily.  In the simplest setup, Owned just effectively does a
>> scoped_ptr<> inside InvokerStorage, and passes the raw pointer to the
>> function on each call.  There is no way for the called function to take
>> ownership of the pointer.  I like this a lot because InvokerStorage is still
>> immutable, and multiple invocations get the same argument values passed in.
>>  However, there critical shortcoming is that you cannot pass ownership.
>>
>
> Ah yes, this sounds nice. I like base::Owned() being used for this. How
> about a separate class for the memory transfer case? Don't do any implicit
> conversions in the class or base::Bind(), so the function prototype has to
> have an instance of this class as a parameter. That'll help document that
> the function is doing a memory transfer and the user needs to be careful.
>
>
>> My thought behind adding the MaybeCapturePtr<> type was that it would tell
>> the person writing the function that they had to make a conscious choice
>> about whether or not to try and take ownership of a pointer.  If they never
>> call arg.release(), then the callback is actually safe to run multiple
>> times.
>>
>
> I think that's too confusing. I'd rather be explicit about the two
> different cases here. One where the Callback takes complete ownership of the
> memory and another where it's simply used for transferring ownership to the
> function we invoke (but is useful to shut up valgrind leaks where the
> Callback doesn't get run). I'd hope the latter case would be less common
> than base::Owned(), so it'd draw more attention when used, and we could use
> debug assertions to try to prevent multiple Run() calls.
>

I think you're right...cutting apart these uses cases does make things
nicer.

How about the following naming:

  base::Owned() == scoped_ptr for Callbacks.
  base::Passed() == the callee had better delete the sucker or do something
smart.

In both cases, the callee takes a raw pointer type, and we disallow
refcounted types.

Sound good?  Any need to support arrays, mallocs, or deleting on the correct
thread? (I'm thinking not until someone really wants it...)

-Albert



>
>
>>
>> -Albert
>>
>>
>>
>>>
>>> It strikes me as something where we could solve it with assertions in the
>>> debug code without bloating the release code. Use assertions to enforce a
>>> callback that has a base::Owned parameter doesn't have Run() called multiple
>>> times.
>>>
>>>
>>> On Sat, Oct 8, 2011 at 11:47 PM, Albert J. Wong (王重傑) <
>>> ajwong@chromium.org> wrote:
>>>
>>>> The main thing that I worry about is multiple runs of a callback.  For
>>>> instance, I think with a semantic where Owned() always released the pointer
>>>> to the bound function, you'd have the following semantics:
>>>>
>>>> void Func(Foo* f) {
>>>>   // We need to remember that ownership for f is passed and delete
>>>> appropriately.
>>>>   f->DoSomething();
>>>>   delete f;
>>>> }
>>>>
>>>> Closure cb = Bind(&Func, Owned(new Foo()));
>>>>
>>>> cb.Run();  // Works fine.
>>>> cb.Run();  // |f| above would be NULL, and f->DoSomething() would crash.
>>>>
>>>> -Albert
>>>>
>>>>
>>>> On Sat, Oct 8, 2011 at 11:40 PM, William Chan (陈智昌) <
>>>> willchan@chromium.org> wrote:
>>>>
>>>>> Can we make it so base::Owned() creates a scoped_ptr<T> in the
>>>>> InvokerStorage? And then, have DoInvoke unwrap all arguments, where
>>>>> unwrapping a scoped_ptr<T> would call .release() on it?
>>>>>
>>>>>
>>>>> On Sat, Oct 8, 2011 at 11:36 PM, <ajwong@chromium.org> wrote:
>>>>>
>>>>>> One last possibility, we could just make this a simple Owned()
>>>>>> concept, and only
>>>>>> pass back raw pointers without anything like MaybeCaptures, or
>>>>>> PassOwnPtr.
>>>>>> Then, if someone wants to "pass ownership" they would do
>>>>>>
>>>>>> void Func(scoped_ptr<Foo>* f);
>>>>>>
>>>>>> base::Bind(&Func, Owned(new scoped_ptr<Foo>(new Foo()));
>>>>>>
>>>>>> That's pretty ugly too though.
>>>>>>
>>>>>>
>>>>>> On 2011/10/08 20:48:07, awong wrote:
>>>>>>
>>>>>>> The thing I'm struggling the most with this design is that
>>>>>>> MaybeCapturePtr
>>>>>>>
>>>>>> feels
>>>>>>
>>>>>>> a lot like PassOwnPtr, but just different enough.
>>>>>>>
>>>>>>
>>>>>>  One could argue that the interface should just work like:
>>>>>>>
>>>>>>
>>>>>>  void Func(PassOwnPtr<Foo> f) {
>>>>>>>   if (f) {
>>>>>>>     delete f.release();
>>>>>>>     cout << "released!\n";
>>>>>>>   } else {
>>>>>>>     cout << "was null.\n";
>>>>>>>   }
>>>>>>> }
>>>>>>>
>>>>>>
>>>>>>  Closure cb = Bind(&Func(PassOwnPtr<new Foo()>));
>>>>>>>
>>>>>>
>>>>>>  cb.Run();  // Prints "released!"
>>>>>>> cb.Run();  // Prints "was null."
>>>>>>>
>>>>>>
>>>>>>  This has less types in the system, but would require PassOwnPtr to be
>>>>>>> much
>>>>>>> smarter than what's in WebKit today (it'd need to have similar
>>>>>>> functionality
>>>>>>>
>>>>>> to
>>>>>>
>>>>>>> MaybeCapturesPtr in my implementation).  Also, the implementor of Foo
>>>>>>> has less
>>>>>>> indication that the PassOwnPtr might come from a callback, and thus
>>>>>>> they might
>>>>>>> need to consider nullness of the argument.
>>>>>>>
>>>>>>
>>>>>>  Thoughts?
>>>>>>>
>>>>>>
>>>>>>  -Albert
>>>>>>>
>>>>>>
>>>>>>
>>>>>>  On 2011/10/08 19:10:57, awong wrote:
>>>>>>> > Here's a proposal for Owned().
>>>>>>> >
>>>>>>> > Please look at the unittest mainly and tell me what you think of
>>>>>>> the usage.
>>>>>>> > There are 2 issues I'd particularly like feedback on.
>>>>>>> >
>>>>>>> >   1) What to name these things?  MaybeCapturePtr sounds awkward.
>>>>>>> >   2) Do you like the ownership transfer semantics of forcing a type
>>>>>>> like
>>>>>>> > MaybeCapturePtr.
>>>>>>> >
>>>>>>> > Also note that htis only works with things that are "new"ed.
>>>>>>>  Doesn't work
>>>>>>> with
>>>>>>> > new[], or malloc.
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
http://codereview.chromium.**org/8209001/<http://codereview.chromium.org/8209...
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

[willchan no longer on Chromium, 2011-10-09 07:38:39]

On Sun, Oct 9, 2011 at 12:23 AM, Albert J. Wong (王重傑)
<ajwong@chromium.org>wrote:

> On Sun, Oct 9, 2011 at 12:16 AM, William Chan (陈智昌) <willchan@chromium.org
> > wrote:
>
>> On Sat, Oct 8, 2011 at 11:57 PM, Albert J. Wong (王重傑) <
>> ajwong@chromium.org> wrote:
>>
>>> On Sat, Oct 8, 2011 at 11:51 PM, William Chan (陈智昌) <
>>> willchan@chromium.org> wrote:
>>>
>>>> Isn't that true for all the proposals with base::Owned()? Anything that
>>>> allows you to transfer ownership means it is unsafe to invoke the callback
>>>> multiple times.
>>>
>>>
>>> Not necessarily.  In the simplest setup, Owned just effectively does a
>>> scoped_ptr<> inside InvokerStorage, and passes the raw pointer to the
>>> function on each call.  There is no way for the called function to take
>>> ownership of the pointer.  I like this a lot because InvokerStorage is still
>>> immutable, and multiple invocations get the same argument values passed in.
>>>  However, there critical shortcoming is that you cannot pass ownership.
>>>
>>
>> Ah yes, this sounds nice. I like base::Owned() being used for this. How
>> about a separate class for the memory transfer case? Don't do any implicit
>> conversions in the class or base::Bind(), so the function prototype has to
>> have an instance of this class as a parameter. That'll help document that
>> the function is doing a memory transfer and the user needs to be careful.
>>
>>
>>> My thought behind adding the MaybeCapturePtr<> type was that it would
>>> tell the person writing the function that they had to make a conscious
>>> choice about whether or not to try and take ownership of a pointer.  If they
>>> never call arg.release(), then the callback is actually safe to run multiple
>>> times.
>>>
>>
>> I think that's too confusing. I'd rather be explicit about the two
>> different cases here. One where the Callback takes complete ownership of the
>> memory and another where it's simply used for transferring ownership to the
>> function we invoke (but is useful to shut up valgrind leaks where the
>> Callback doesn't get run). I'd hope the latter case would be less common
>> than base::Owned(), so it'd draw more attention when used, and we could use
>> debug assertions to try to prevent multiple Run() calls.
>>
>
> I think you're right...cutting apart these uses cases does make things
> nicer.
>
> How about the following naming:
>
>   base::Owned() == scoped_ptr for Callbacks.
>   base::Passed() == the callee had better delete the sucker or do something
> smart.
>
> In both cases, the callee takes a raw pointer type, and we disallow
> refcounted types.
>

I'm unsure if we should be a bit paranoid/annoying and for the latter case
require a Passed<T> type that does not do implicit conversions. I'm leaning
towards being annoying. I expect base::Passed() to be primarily used for the
case where you want to transfer ownership to an object, in which case that
object's member function can use a Passed<T> parameter to make this
explicit.


>
> Sound good?  Any need to support arrays, mallocs, or deleting on the
> correct thread? (I'm thinking not until someone really wants it...)
>
> -Albert
>
>
>
>>
>>
>>>
>>> -Albert
>>>
>>>
>>>
>>>>
>>>> It strikes me as something where we could solve it with assertions in
>>>> the debug code without bloating the release code. Use assertions to enforce
>>>> a callback that has a base::Owned parameter doesn't have Run() called
>>>> multiple times.
>>>>
>>>>
>>>> On Sat, Oct 8, 2011 at 11:47 PM, Albert J. Wong (王重傑) <
>>>> ajwong@chromium.org> wrote:
>>>>
>>>>> The main thing that I worry about is multiple runs of a callback.  For
>>>>> instance, I think with a semantic where Owned() always released the
pointer
>>>>> to the bound function, you'd have the following semantics:
>>>>>
>>>>> void Func(Foo* f) {
>>>>>   // We need to remember that ownership for f is passed and delete
>>>>> appropriately.
>>>>>   f->DoSomething();
>>>>>   delete f;
>>>>> }
>>>>>
>>>>> Closure cb = Bind(&Func, Owned(new Foo()));
>>>>>
>>>>> cb.Run();  // Works fine.
>>>>> cb.Run();  // |f| above would be NULL, and f->DoSomething() would
>>>>> crash.
>>>>>
>>>>> -Albert
>>>>>
>>>>>
>>>>> On Sat, Oct 8, 2011 at 11:40 PM, William Chan (陈智昌) <
>>>>> willchan@chromium.org> wrote:
>>>>>
>>>>>> Can we make it so base::Owned() creates a scoped_ptr<T> in the
>>>>>> InvokerStorage? And then, have DoInvoke unwrap all arguments, where
>>>>>> unwrapping a scoped_ptr<T> would call .release() on it?
>>>>>>
>>>>>>
>>>>>> On Sat, Oct 8, 2011 at 11:36 PM, <ajwong@chromium.org> wrote:
>>>>>>
>>>>>>> One last possibility, we could just make this a simple Owned()
>>>>>>> concept, and only
>>>>>>> pass back raw pointers without anything like MaybeCaptures, or
>>>>>>> PassOwnPtr.
>>>>>>> Then, if someone wants to "pass ownership" they would do
>>>>>>>
>>>>>>> void Func(scoped_ptr<Foo>* f);
>>>>>>>
>>>>>>> base::Bind(&Func, Owned(new scoped_ptr<Foo>(new Foo()));
>>>>>>>
>>>>>>> That's pretty ugly too though.
>>>>>>>
>>>>>>>
>>>>>>> On 2011/10/08 20:48:07, awong wrote:
>>>>>>>
>>>>>>>> The thing I'm struggling the most with this design is that
>>>>>>>> MaybeCapturePtr
>>>>>>>>
>>>>>>> feels
>>>>>>>
>>>>>>>> a lot like PassOwnPtr, but just different enough.
>>>>>>>>
>>>>>>>
>>>>>>>  One could argue that the interface should just work like:
>>>>>>>>
>>>>>>>
>>>>>>>  void Func(PassOwnPtr<Foo> f) {
>>>>>>>>   if (f) {
>>>>>>>>     delete f.release();
>>>>>>>>     cout << "released!\n";
>>>>>>>>   } else {
>>>>>>>>     cout << "was null.\n";
>>>>>>>>   }
>>>>>>>> }
>>>>>>>>
>>>>>>>
>>>>>>>  Closure cb = Bind(&Func(PassOwnPtr<new Foo()>));
>>>>>>>>
>>>>>>>
>>>>>>>  cb.Run();  // Prints "released!"
>>>>>>>> cb.Run();  // Prints "was null."
>>>>>>>>
>>>>>>>
>>>>>>>  This has less types in the system, but would require PassOwnPtr to
>>>>>>>> be much
>>>>>>>> smarter than what's in WebKit today (it'd need to have similar
>>>>>>>> functionality
>>>>>>>>
>>>>>>> to
>>>>>>>
>>>>>>>> MaybeCapturesPtr in my implementation).  Also, the implementor of
>>>>>>>> Foo has less
>>>>>>>> indication that the PassOwnPtr might come from a callback, and thus
>>>>>>>> they might
>>>>>>>> need to consider nullness of the argument.
>>>>>>>>
>>>>>>>
>>>>>>>  Thoughts?
>>>>>>>>
>>>>>>>
>>>>>>>  -Albert
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>  On 2011/10/08 19:10:57, awong wrote:
>>>>>>>> > Here's a proposal for Owned().
>>>>>>>> >
>>>>>>>> > Please look at the unittest mainly and tell me what you think of
>>>>>>>> the usage.
>>>>>>>> > There are 2 issues I'd particularly like feedback on.
>>>>>>>> >
>>>>>>>> >   1) What to name these things?  MaybeCapturePtr sounds awkward.
>>>>>>>> >   2) Do you like the ownership transfer semantics of forcing a
>>>>>>>> type like
>>>>>>>> > MaybeCapturePtr.
>>>>>>>> >
>>>>>>>> > Also note that htis only works with things that are "new"ed.
>>>>>>>>  Doesn't work
>>>>>>>> with
>>>>>>>> > new[], or malloc.
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
http://codereview.chromium.**org/8209001/<http://codereview.chromium.org/8209...
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

[awong, 2011-10-09 07:45:03]

On Sun, Oct 9, 2011 at 12:38 AM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> On Sun, Oct 9, 2011 at 12:23 AM, Albert J. Wong (王重傑) <ajwong@chromium.org
> > wrote:
>
>> On Sun, Oct 9, 2011 at 12:16 AM, William Chan (陈智昌) <
>> willchan@chromium.org> wrote:
>>
>>> On Sat, Oct 8, 2011 at 11:57 PM, Albert J. Wong (王重傑) <
>>> ajwong@chromium.org> wrote:
>>>
>>>> On Sat, Oct 8, 2011 at 11:51 PM, William Chan (陈智昌) <
>>>> willchan@chromium.org> wrote:
>>>>
>>>>> Isn't that true for all the proposals with base::Owned()? Anything that
>>>>> allows you to transfer ownership means it is unsafe to invoke the callback
>>>>> multiple times.
>>>>
>>>>
>>>> Not necessarily.  In the simplest setup, Owned just effectively does a
>>>> scoped_ptr<> inside InvokerStorage, and passes the raw pointer to the
>>>> function on each call.  There is no way for the called function to take
>>>> ownership of the pointer.  I like this a lot because InvokerStorage is
still
>>>> immutable, and multiple invocations get the same argument values passed in.
>>>>  However, there critical shortcoming is that you cannot pass ownership.
>>>>
>>>
>>> Ah yes, this sounds nice. I like base::Owned() being used for this. How
>>> about a separate class for the memory transfer case? Don't do any implicit
>>> conversions in the class or base::Bind(), so the function prototype has to
>>> have an instance of this class as a parameter. That'll help document that
>>> the function is doing a memory transfer and the user needs to be careful.
>>>
>>>
>>>> My thought behind adding the MaybeCapturePtr<> type was that it would
>>>> tell the person writing the function that they had to make a conscious
>>>> choice about whether or not to try and take ownership of a pointer.  If
they
>>>> never call arg.release(), then the callback is actually safe to run
multiple
>>>> times.
>>>>
>>>
>>> I think that's too confusing. I'd rather be explicit about the two
>>> different cases here. One where the Callback takes complete ownership of the
>>> memory and another where it's simply used for transferring ownership to the
>>> function we invoke (but is useful to shut up valgrind leaks where the
>>> Callback doesn't get run). I'd hope the latter case would be less common
>>> than base::Owned(), so it'd draw more attention when used, and we could use
>>> debug assertions to try to prevent multiple Run() calls.
>>>
>>
>> I think you're right...cutting apart these uses cases does make things
>> nicer.
>>
>> How about the following naming:
>>
>>   base::Owned() == scoped_ptr for Callbacks.
>>   base::Passed() == the callee had better delete the sucker or do
>> something smart.
>>
>> In both cases, the callee takes a raw pointer type, and we disallow
>> refcounted types.
>>
>
> I'm unsure if we should be a bit paranoid/annoying and for the latter case
> require a Passed<T> type that does not do implicit conversions. I'm leaning
> towards being annoying. I expect base::Passed() to be primarily used for the
> case where you want to transfer ownership to an object, in which case that
> object's member function can use a Passed<T> parameter to make this
> explicit.
>

This is getting very close to introducing a wholesale auto_ptr/PassOwnPtr
equivalent where you have destructive copy semantics, etc.  If you had such
a thing, I think things may "just work" as is.

I've been shying away from adding PassOwnPtr cause I remember hearing some
resistance about it.  But maybe it's time to try adding it formally?

-Albert



>
>
>>
>> Sound good?  Any need to support arrays, mallocs, or deleting on the
>> correct thread? (I'm thinking not until someone really wants it...)
>>
>> -Albert
>>
>>
>>
>>>
>>>
>>>>
>>>> -Albert
>>>>
>>>>
>>>>
>>>>>
>>>>> It strikes me as something where we could solve it with assertions in
>>>>> the debug code without bloating the release code. Use assertions to
enforce
>>>>> a callback that has a base::Owned parameter doesn't have Run() called
>>>>> multiple times.
>>>>>
>>>>>
>>>>> On Sat, Oct 8, 2011 at 11:47 PM, Albert J. Wong (王重傑) <
>>>>> ajwong@chromium.org> wrote:
>>>>>
>>>>>> The main thing that I worry about is multiple runs of a callback.  For
>>>>>> instance, I think with a semantic where Owned() always released the
pointer
>>>>>> to the bound function, you'd have the following semantics:
>>>>>>
>>>>>> void Func(Foo* f) {
>>>>>>   // We need to remember that ownership for f is passed and delete
>>>>>> appropriately.
>>>>>>   f->DoSomething();
>>>>>>   delete f;
>>>>>> }
>>>>>>
>>>>>> Closure cb = Bind(&Func, Owned(new Foo()));
>>>>>>
>>>>>> cb.Run();  // Works fine.
>>>>>> cb.Run();  // |f| above would be NULL, and f->DoSomething() would
>>>>>> crash.
>>>>>>
>>>>>> -Albert
>>>>>>
>>>>>>
>>>>>> On Sat, Oct 8, 2011 at 11:40 PM, William Chan (陈智昌) <
>>>>>> willchan@chromium.org> wrote:
>>>>>>
>>>>>>> Can we make it so base::Owned() creates a scoped_ptr<T> in the
>>>>>>> InvokerStorage? And then, have DoInvoke unwrap all arguments, where
>>>>>>> unwrapping a scoped_ptr<T> would call .release() on it?
>>>>>>>
>>>>>>>
>>>>>>> On Sat, Oct 8, 2011 at 11:36 PM, <ajwong@chromium.org> wrote:
>>>>>>>
>>>>>>>> One last possibility, we could just make this a simple Owned()
>>>>>>>> concept, and only
>>>>>>>> pass back raw pointers without anything like MaybeCaptures, or
>>>>>>>> PassOwnPtr.
>>>>>>>> Then, if someone wants to "pass ownership" they would do
>>>>>>>>
>>>>>>>> void Func(scoped_ptr<Foo>* f);
>>>>>>>>
>>>>>>>> base::Bind(&Func, Owned(new scoped_ptr<Foo>(new Foo()));
>>>>>>>>
>>>>>>>> That's pretty ugly too though.
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2011/10/08 20:48:07, awong wrote:
>>>>>>>>
>>>>>>>>> The thing I'm struggling the most with this design is that
>>>>>>>>> MaybeCapturePtr
>>>>>>>>>
>>>>>>>> feels
>>>>>>>>
>>>>>>>>> a lot like PassOwnPtr, but just different enough.
>>>>>>>>>
>>>>>>>>
>>>>>>>>  One could argue that the interface should just work like:
>>>>>>>>>
>>>>>>>>
>>>>>>>>  void Func(PassOwnPtr<Foo> f) {
>>>>>>>>>   if (f) {
>>>>>>>>>     delete f.release();
>>>>>>>>>     cout << "released!\n";
>>>>>>>>>   } else {
>>>>>>>>>     cout << "was null.\n";
>>>>>>>>>   }
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>
>>>>>>>>  Closure cb = Bind(&Func(PassOwnPtr<new Foo()>));
>>>>>>>>>
>>>>>>>>
>>>>>>>>  cb.Run();  // Prints "released!"
>>>>>>>>> cb.Run();  // Prints "was null."
>>>>>>>>>
>>>>>>>>
>>>>>>>>  This has less types in the system, but would require PassOwnPtr to
>>>>>>>>> be much
>>>>>>>>> smarter than what's in WebKit today (it'd need to have similar
>>>>>>>>> functionality
>>>>>>>>>
>>>>>>>> to
>>>>>>>>
>>>>>>>>> MaybeCapturesPtr in my implementation).  Also, the implementor of
>>>>>>>>> Foo has less
>>>>>>>>> indication that the PassOwnPtr might come from a callback, and thus
>>>>>>>>> they might
>>>>>>>>> need to consider nullness of the argument.
>>>>>>>>>
>>>>>>>>
>>>>>>>>  Thoughts?
>>>>>>>>>
>>>>>>>>
>>>>>>>>  -Albert
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>  On 2011/10/08 19:10:57, awong wrote:
>>>>>>>>> > Here's a proposal for Owned().
>>>>>>>>> >
>>>>>>>>> > Please look at the unittest mainly and tell me what you think of
>>>>>>>>> the usage.
>>>>>>>>> > There are 2 issues I'd particularly like feedback on.
>>>>>>>>> >
>>>>>>>>> >   1) What to name these things?  MaybeCapturePtr sounds awkward.
>>>>>>>>> >   2) Do you like the ownership transfer semantics of forcing a
>>>>>>>>> type like
>>>>>>>>> > MaybeCapturePtr.
>>>>>>>>> >
>>>>>>>>> > Also note that htis only works with things that are "new"ed.
>>>>>>>>>  Doesn't work
>>>>>>>>> with
>>>>>>>>> > new[], or malloc.
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
http://codereview.chromium.**org/8209001/<http://codereview.chromium.org/8209...
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

[willchan no longer on Chromium, 2011-10-09 18:09:12]

On Sun, Oct 9, 2011 at 12:44 AM, Albert J. Wong (王重傑)
<ajwong@chromium.org>wrote:

>
>
> On Sun, Oct 9, 2011 at 12:38 AM, William Chan (陈智昌) <willchan@chromium.org
> > wrote:
>
>> On Sun, Oct 9, 2011 at 12:23 AM, Albert J. Wong (王重傑) <
>> ajwong@chromium.org> wrote:
>>
>>> On Sun, Oct 9, 2011 at 12:16 AM, William Chan (陈智昌) <
>>> willchan@chromium.org> wrote:
>>>
>>>> On Sat, Oct 8, 2011 at 11:57 PM, Albert J. Wong (王重傑) <
>>>> ajwong@chromium.org> wrote:
>>>>
>>>>> On Sat, Oct 8, 2011 at 11:51 PM, William Chan (陈智昌) <
>>>>> willchan@chromium.org> wrote:
>>>>>
>>>>>> Isn't that true for all the proposals with base::Owned()? Anything
>>>>>> that allows you to transfer ownership means it is unsafe to invoke the
>>>>>> callback multiple times.
>>>>>
>>>>>
>>>>> Not necessarily.  In the simplest setup, Owned just effectively does a
>>>>> scoped_ptr<> inside InvokerStorage, and passes the raw pointer to the
>>>>> function on each call.  There is no way for the called function to take
>>>>> ownership of the pointer.  I like this a lot because InvokerStorage is
still
>>>>> immutable, and multiple invocations get the same argument values passed
in.
>>>>>  However, there critical shortcoming is that you cannot pass ownership.
>>>>>
>>>>
>>>> Ah yes, this sounds nice. I like base::Owned() being used for this. How
>>>> about a separate class for the memory transfer case? Don't do any implicit
>>>> conversions in the class or base::Bind(), so the function prototype has to
>>>> have an instance of this class as a parameter. That'll help document that
>>>> the function is doing a memory transfer and the user needs to be careful.
>>>>
>>>>
>>>>> My thought behind adding the MaybeCapturePtr<> type was that it would
>>>>> tell the person writing the function that they had to make a conscious
>>>>> choice about whether or not to try and take ownership of a pointer.  If
they
>>>>> never call arg.release(), then the callback is actually safe to run
multiple
>>>>> times.
>>>>>
>>>>
>>>> I think that's too confusing. I'd rather be explicit about the two
>>>> different cases here. One where the Callback takes complete ownership of
the
>>>> memory and another where it's simply used for transferring ownership to the
>>>> function we invoke (but is useful to shut up valgrind leaks where the
>>>> Callback doesn't get run). I'd hope the latter case would be less common
>>>> than base::Owned(), so it'd draw more attention when used, and we could use
>>>> debug assertions to try to prevent multiple Run() calls.
>>>>
>>>
>>> I think you're right...cutting apart these uses cases does make things
>>> nicer.
>>>
>>> How about the following naming:
>>>
>>>   base::Owned() == scoped_ptr for Callbacks.
>>>   base::Passed() == the callee had better delete the sucker or do
>>> something smart.
>>>
>>> In both cases, the callee takes a raw pointer type, and we disallow
>>> refcounted types.
>>>
>>
>> I'm unsure if we should be a bit paranoid/annoying and for the latter case
>> require a Passed<T> type that does not do implicit conversions. I'm leaning
>> towards being annoying. I expect base::Passed() to be primarily used for the
>> case where you want to transfer ownership to an object, in which case that
>> object's member function can use a Passed<T> parameter to make this
>> explicit.
>>
>
> This is getting very close to introducing a wholesale auto_ptr/PassOwnPtr
> equivalent where you have destructive copy semantics, etc.  If you had such
> a thing, I think things may "just work" as is.
>
> I've been shying away from adding PassOwnPtr cause I remember hearing some
> resistance about it.  But maybe it's time to try adding it formally?
>

Ah, I didn't know about the resistance. I guess it makes sense, since indeed
we disallow auto_ptr in the style guide. This is the only place I can
conceive of using it, but I don't know if we can prevent people from abusing
it once we introduce it. Maybe it indeed is better to just use raw pointers
in both cases. I am ambivalent. I think I'll defer to Darin here since he
has more experience with WTF::PassOwnPtr<T> anyway.


>
> -Albert
>
>
>
>>
>>
>>>
>>> Sound good?  Any need to support arrays, mallocs, or deleting on the
>>> correct thread? (I'm thinking not until someone really wants it...)
>>>
>>> -Albert
>>>
>>>
>>>
>>>>
>>>>
>>>>>
>>>>> -Albert
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> It strikes me as something where we could solve it with assertions in
>>>>>> the debug code without bloating the release code. Use assertions to
enforce
>>>>>> a callback that has a base::Owned parameter doesn't have Run() called
>>>>>> multiple times.
>>>>>>
>>>>>>
>>>>>> On Sat, Oct 8, 2011 at 11:47 PM, Albert J. Wong (王重傑) <
>>>>>> ajwong@chromium.org> wrote:
>>>>>>
>>>>>>> The main thing that I worry about is multiple runs of a callback.
>>>>>>>  For instance, I think with a semantic where Owned() always released the
>>>>>>> pointer to the bound function, you'd have the following semantics:
>>>>>>>
>>>>>>> void Func(Foo* f) {
>>>>>>>   // We need to remember that ownership for f is passed and delete
>>>>>>> appropriately.
>>>>>>>   f->DoSomething();
>>>>>>>   delete f;
>>>>>>> }
>>>>>>>
>>>>>>> Closure cb = Bind(&Func, Owned(new Foo()));
>>>>>>>
>>>>>>> cb.Run();  // Works fine.
>>>>>>> cb.Run();  // |f| above would be NULL, and f->DoSomething() would
>>>>>>> crash.
>>>>>>>
>>>>>>> -Albert
>>>>>>>
>>>>>>>
>>>>>>> On Sat, Oct 8, 2011 at 11:40 PM, William Chan (陈智昌) <
>>>>>>> willchan@chromium.org> wrote:
>>>>>>>
>>>>>>>> Can we make it so base::Owned() creates a scoped_ptr<T> in the
>>>>>>>> InvokerStorage? And then, have DoInvoke unwrap all arguments, where
>>>>>>>> unwrapping a scoped_ptr<T> would call .release() on it?
>>>>>>>>
>>>>>>>>
>>>>>>>> On Sat, Oct 8, 2011 at 11:36 PM, <ajwong@chromium.org> wrote:
>>>>>>>>
>>>>>>>>> One last possibility, we could just make this a simple Owned()
>>>>>>>>> concept, and only
>>>>>>>>> pass back raw pointers without anything like MaybeCaptures, or
>>>>>>>>> PassOwnPtr.
>>>>>>>>> Then, if someone wants to "pass ownership" they would do
>>>>>>>>>
>>>>>>>>> void Func(scoped_ptr<Foo>* f);
>>>>>>>>>
>>>>>>>>> base::Bind(&Func, Owned(new scoped_ptr<Foo>(new Foo()));
>>>>>>>>>
>>>>>>>>> That's pretty ugly too though.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2011/10/08 20:48:07, awong wrote:
>>>>>>>>>
>>>>>>>>>> The thing I'm struggling the most with this design is that
>>>>>>>>>> MaybeCapturePtr
>>>>>>>>>>
>>>>>>>>> feels
>>>>>>>>>
>>>>>>>>>> a lot like PassOwnPtr, but just different enough.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>  One could argue that the interface should just work like:
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>  void Func(PassOwnPtr<Foo> f) {
>>>>>>>>>>   if (f) {
>>>>>>>>>>     delete f.release();
>>>>>>>>>>     cout << "released!\n";
>>>>>>>>>>   } else {
>>>>>>>>>>     cout << "was null.\n";
>>>>>>>>>>   }
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>  Closure cb = Bind(&Func(PassOwnPtr<new Foo()>));
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>  cb.Run();  // Prints "released!"
>>>>>>>>>> cb.Run();  // Prints "was null."
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>  This has less types in the system, but would require PassOwnPtr to
>>>>>>>>>> be much
>>>>>>>>>> smarter than what's in WebKit today (it'd need to have similar
>>>>>>>>>> functionality
>>>>>>>>>>
>>>>>>>>> to
>>>>>>>>>
>>>>>>>>>> MaybeCapturesPtr in my implementation).  Also, the implementor of
>>>>>>>>>> Foo has less
>>>>>>>>>> indication that the PassOwnPtr might come from a callback, and
>>>>>>>>>> thus they might
>>>>>>>>>> need to consider nullness of the argument.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>  Thoughts?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>  -Albert
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>  On 2011/10/08 19:10:57, awong wrote:
>>>>>>>>>> > Here's a proposal for Owned().
>>>>>>>>>> >
>>>>>>>>>> > Please look at the unittest mainly and tell me what you think of
>>>>>>>>>> the usage.
>>>>>>>>>> > There are 2 issues I'd particularly like feedback on.
>>>>>>>>>> >
>>>>>>>>>> >   1) What to name these things?  MaybeCapturePtr sounds awkward.
>>>>>>>>>> >   2) Do you like the ownership transfer semantics of forcing a
>>>>>>>>>> type like
>>>>>>>>>> > MaybeCapturePtr.
>>>>>>>>>> >
>>>>>>>>>> > Also note that htis only works with things that are "new"ed.
>>>>>>>>>>  Doesn't work
>>>>>>>>>> with
>>>>>>>>>> > new[], or malloc.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
http://codereview.chromium.**org/8209001/<http://codereview.chromium.org/8209...
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

[darin (slow to review), 2011-10-11 05:13:49]

http://codereview.chromium.org/8209001/diff/8002/base/bind_helpers.h
File base/bind_helpers.h (right):

http://codereview.chromium.org/8209001/diff/8002/base/bind_helpers.h#newcode335
base/bind_helpers.h:335: inline internal::OwnedWrapper<T> Owned(T* o) {
how about some documentation?  i'm concerned that this system is growing
increasingly complicated.

i also wish there was some consistency to how these coercion helpers were named
:-)

[awong, 2011-10-11 17:24:27]

PTAL

http://codereview.chromium.org/8209001/diff/8002/base/bind_helpers.h
File base/bind_helpers.h (right):

http://codereview.chromium.org/8209001/diff/8002/base/bind_helpers.h#newcode335
base/bind_helpers.h:335: inline internal::OwnedWrapper<T> Owned(T* o) {
On 2011/10/11 05:13:50, darin wrote:
> how about some documentation?  i'm concerned that this system is growing
> increasingly complicated.

This is the start of concern?  Or are you concerned that it is already too
complicated?

I think the endpoint of features will be:
  1) Solving ownership (probably just Owned())
  2) Solving cancellation.

The second one, I expect, to be some sort of wrapper on Callback.

Both these use cases I've been seeing concrete needs for in the CLs I've been
reviewing.

Does that sound too complicated?  I do agree with the general worry though.
 
> i also wish there was some consistency to how these coercion helpers were
named
> :-)

Hmm...you mean that no one has a way to know that Unretained(), ConstRef(),
IgnoreResult(), and Owned() have anything to do with one another?

I supposed we could add a prefix, but I shy away from a making the names longer
cause they are already a bit unwieldy.  We could also dump them into a
namespace, but that will pragmatically cause a similar problem since people
don't often use using statements...

-Albert

[awong, 2011-10-12 08:31:05]

ping-a-ling

On 2011/10/11 17:24:27, awong wrote:
> PTAL
> 
> http://codereview.chromium.org/8209001/diff/8002/base/bind_helpers.h
> File base/bind_helpers.h (right):
> 
>
http://codereview.chromium.org/8209001/diff/8002/base/bind_helpers.h#newcode335
> base/bind_helpers.h:335: inline internal::OwnedWrapper<T> Owned(T* o) {
> On 2011/10/11 05:13:50, darin wrote:
> > how about some documentation?  i'm concerned that this system is growing
> > increasingly complicated.
> 
> This is the start of concern?  Or are you concerned that it is already too
> complicated?
> 
> I think the endpoint of features will be:
>   1) Solving ownership (probably just Owned())
>   2) Solving cancellation.
> 
> The second one, I expect, to be some sort of wrapper on Callback.
> 
> Both these use cases I've been seeing concrete needs for in the CLs I've been
> reviewing.
> 
> Does that sound too complicated?  I do agree with the general worry though.
>  
> > i also wish there was some consistency to how these coercion helpers were
> named
> > :-)
> 
> Hmm...you mean that no one has a way to know that Unretained(), ConstRef(),
> IgnoreResult(), and Owned() have anything to do with one another?
> 
> I supposed we could add a prefix, but I shy away from a making the names
longer
> cause they are already a bit unwieldy.  We could also dump them into a
> namespace, but that will pragmatically cause a similar problem since people
> don't often use using statements...
> 
> -Albert

[darin (slow to review), 2011-10-14 21:53:07]

LGTM

http://codereview.chromium.org/8209001/diff/11001/base/bind_helpers.h
File base/bind_helpers.h (right):

http://codereview.chromium.org/8209001/diff/11001/base/bind_helpers.h#newcode40
base/bind_helpers.h:40: // EXAMPLE OF Owned():
nit: new line below this line as you did for Unretained(), or kill the new line
for the Unretained case.

http://codereview.chromium.org/8209001/diff/11001/base/bind_helpers.h#newcode242
base/bind_helpers.h:242: // This has the benefit though of leaving ParamTrais<>
fully in
nit: ParamTrais -> ParamTraits

[awong, 2011-10-15 00:19:16]

http://codereview.chromium.org/8209001/diff/11001/base/bind_helpers.h
File base/bind_helpers.h (right):

http://codereview.chromium.org/8209001/diff/11001/base/bind_helpers.h#newcode40
base/bind_helpers.h:40: // EXAMPLE OF Owned():
On 2011/10/14 21:53:07, darin wrote:
> nit: new line below this line as you did for Unretained(), or kill the new
line
> for the Unretained case.

Done.  Also fixed ConstRef and IgnoreReturn.

http://codereview.chromium.org/8209001/diff/11001/base/bind_helpers.h#newcode242
base/bind_helpers.h:242: // This has the benefit though of leaving ParamTrais<>
fully in
On 2011/10/14 21:53:07, darin wrote:
> nit: ParamTrais -> ParamTraits

Done.