Add a PassScopedPtr into base/memory.

Add a PassScopedPtr into base/memory.

This can be used in conjunction with base::Bind() to pass ownership of data into a bound Callback.

The class itself has no thread safety.  It requires external synchronization.

BUG=96118
TEST=new unittests.

[awong, 2011-10-11 02:51:20]

Darin, I think you might be best for this review since it's a potentially much
more viral change as it adds another smart pointer type.

Dave, you're on it cause you're the one I talked to about this the most. :)

Though it shares the same name as PassOwnPtr in WebKit, the is a "cleanroom"
implementation forked off of the scoped_ptr implementation.

Let me know what you think.

An alternate approach is to add a limited "base::Passed()" type similar to
base::Owned() here http://codereview.chromium.org/8209001/ that can only work
with base::Bind().

Let me know what method you prefer.

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

There are some gotchas with WebKit's PassOwnPtr that may be worth reviewing.
 Dave Levin may have some gems to share.

The one I can think of goes like this:

class Foo {
 private:
  scoped_ptr<X> blah_;
 public:
  void DoStuff(const PassOwnPtr<X>& param) {
    blah_ = param;
    UseParam(param);  // Oops, param is now null because assignment to blah_
stole the pointer!
  }
};

This is a surprisingly common error.  Often, there is a lot of distance
between the blah_ assignment and the UseParam call, and the UseParam call
probably was written before the blah_ assignment.

The key annoyance here is that the error is only really detectable at
run-time :-(

I'm not citing this to say that we shouldn't have PassOwnPtr.  I'm just
wondering....

-Darin


On Mon, Oct 10, 2011 at 7:51 PM, <ajwong@chromium.org> wrote:

> Reviewers: darin, levin,
>
> Message:
> Darin, I think you might be best for this review since it's a potentially
> much
> more viral change as it adds another smart pointer type.
>
> Dave, you're on it cause you're the one I talked to about this the most. :)
>
> Though it shares the same name as PassOwnPtr in WebKit, the is a
> "cleanroom"
> implementation forked off of the scoped_ptr implementation.
>
> Let me know what you think.
>
> An alternate approach is to add a limited "base::Passed()" type similar to
> base::Owned() here
http://codereview.chromium.**org/8209001/<http://codereview.chromium.org/8209...
can only work
> with base::Bind().
>
> Let me know what method you prefer.
>
> Description:
> Add a PassOwnPtr into base/memory.
>
> This can be used in conjunction with base::Bind() to pass ownership of data
> into
> a bound Callback.
>
> The class itself has no thread safety.  It requires external
> synchronization.
>
> BUG=96118
> TEST=new unittests.
>
>
> Please review this at
http://codereview.chromium.**org/8224026/<http://codereview.chromium.org/8224...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M base/base.gyp
>  M base/base.gypi
>  M base/bind_unittest.cc
>  A base/memory/pass_own_ptr.h
>  A base/memory/pass_own_ptr_**unittest.cc
>
>
>

[willchan no longer on Chromium, 2011-10-11 05:09:52]

Darin, have you kept up to date with the other threads wrt base::Owned() and
everything? In case you want to refresh on the use cases, they're at
http://code.google.com/p/chromium/issues/detail?id=96118. I am in favor of a
base::Owned() that makes the Callback take ownership of the memory, and a
base::Pass() that transfers ownership to the Callback, until it invokes the
function pointer, at which point the memory is transferred to the receiving
function. I am torn about whether or not that receiving function's prototype
should have a raw pointer for the transferred memory, or a PassOwnPtr.

What are your thoughts here? I don't have enough experience with the
downsides of PassOwnPtr since all codebases I've worked with extensively
before have outlawed auto_ptr type stuff. If we don't use PassOwnPtr, I feel
like a raw pointer is not obvious enough that memory transfer is occurring.
Anyway, I defer to you, but I guess I'm now leaning against PassOwnPtr.

On Mon, Oct 10, 2011 at 10:01 PM, Darin Fisher <darin@chromium.org> wrote:

> There are some gotchas with WebKit's PassOwnPtr that may be worth
> reviewing.  Dave Levin may have some gems to share.
>
> The one I can think of goes like this:
>
> class Foo {
>  private:
>   scoped_ptr<X> blah_;
>  public:
>   void DoStuff(const PassOwnPtr<X>& param) {
>     blah_ = param;
>     UseParam(param);  // Oops, param is now null because assignment to
> blah_ stole the pointer!
>   }
> };
>
> This is a surprisingly common error.  Often, there is a lot of distance
> between the blah_ assignment and the UseParam call, and the UseParam call
> probably was written before the blah_ assignment.
>
> The key annoyance here is that the error is only really detectable at
> run-time :-(
>
> I'm not citing this to say that we shouldn't have PassOwnPtr.  I'm just
> wondering....
>
> -Darin
>
>
> On Mon, Oct 10, 2011 at 7:51 PM, <ajwong@chromium.org> wrote:
>
>> Reviewers: darin, levin,
>>
>> Message:
>> Darin, I think you might be best for this review since it's a potentially
>> much
>> more viral change as it adds another smart pointer type.
>>
>> Dave, you're on it cause you're the one I talked to about this the most.
>> :)
>>
>> Though it shares the same name as PassOwnPtr in WebKit, the is a
>> "cleanroom"
>> implementation forked off of the scoped_ptr implementation.
>>
>> Let me know what you think.
>>
>> An alternate approach is to add a limited "base::Passed()" type similar to
>> base::Owned() here
http://codereview.chromium.**org/8209001/<http://codereview.chromium.org/8209...
can only work
>> with base::Bind().
>>
>> Let me know what method you prefer.
>>
>> Description:
>> Add a PassOwnPtr into base/memory.
>>
>> This can be used in conjunction with base::Bind() to pass ownership of
>> data into
>> a bound Callback.
>>
>> The class itself has no thread safety.  It requires external
>> synchronization.
>>
>> BUG=96118
>> TEST=new unittests.
>>
>>
>> Please review this at
http://codereview.chromium.**org/8224026/<http://codereview.chromium.org/8224...
>>
>> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>>
>> Affected files:
>>  M base/base.gyp
>>  M base/base.gypi
>>  M base/bind_unittest.cc
>>  A base/memory/pass_own_ptr.h
>>  A base/memory/pass_own_ptr_**unittest.cc
>>
>>
>>
>

[darin (slow to review), 2011-10-11 05:41:19]

Hmm...

base::Owned() makes a lot of sense, although I might consider naming it
Retained() instead.  It is the opposite of Unretained().  Retained() could
work with something that is either RefCounted{ThreadSafe} or a raw pointer.
 If it is a raw pointer, then its lifetime would be bound to the lifetime of
the callback.

The base::PassOwn() idea is interesting, but since it is coupled with a
MaybeCapturePtr<T>, it ends up seeming a lot like PassOwnPtr.  Am I wrong
about that?

By the way, you might find base/platform_file.h interesting.  It has a
PassPlatformFile class.  It works somewhat similarly.

On one hand, I'm a bit surprised we have gone this far without needing
PassOwnPtr.  It does make some things cleaner in WebKit.  But, it isn't
without its pitfalls.  How far would we get with something like Retained()?
 Are there some real transfer-of-ownership use cases you are trying to
address that requires PassOwnPtr?

-Darin



On Mon, Oct 10, 2011 at 10:09 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> Darin, have you kept up to date with the other threads wrt base::Owned()
> and everything? In case you want to refresh on the use cases, they're at
> http://code.google.com/p/chromium/issues/detail?id=96118. I am in favor of
> a base::Owned() that makes the Callback take ownership of the memory, and a
> base::Pass() that transfers ownership to the Callback, until it invokes the
> function pointer, at which point the memory is transferred to the receiving
> function. I am torn about whether or not that receiving function's prototype
> should have a raw pointer for the transferred memory, or a PassOwnPtr.
>
> What are your thoughts here? I don't have enough experience with the
> downsides of PassOwnPtr since all codebases I've worked with extensively
> before have outlawed auto_ptr type stuff. If we don't use PassOwnPtr, I feel
> like a raw pointer is not obvious enough that memory transfer is occurring.
> Anyway, I defer to you, but I guess I'm now leaning against PassOwnPtr.
>
> On Mon, Oct 10, 2011 at 10:01 PM, Darin Fisher <darin@chromium.org> wrote:
>
>> There are some gotchas with WebKit's PassOwnPtr that may be worth
>> reviewing.  Dave Levin may have some gems to share.
>>
>> The one I can think of goes like this:
>>
>> class Foo {
>>  private:
>>   scoped_ptr<X> blah_;
>>  public:
>>   void DoStuff(const PassOwnPtr<X>& param) {
>>     blah_ = param;
>>     UseParam(param);  // Oops, param is now null because assignment to
>> blah_ stole the pointer!
>>   }
>> };
>>
>> This is a surprisingly common error.  Often, there is a lot of distance
>> between the blah_ assignment and the UseParam call, and the UseParam call
>> probably was written before the blah_ assignment.
>>
>> The key annoyance here is that the error is only really detectable at
>> run-time :-(
>>
>> I'm not citing this to say that we shouldn't have PassOwnPtr.  I'm just
>> wondering....
>>
>> -Darin
>>
>>
>> On Mon, Oct 10, 2011 at 7:51 PM, <ajwong@chromium.org> wrote:
>>
>>> Reviewers: darin, levin,
>>>
>>> Message:
>>> Darin, I think you might be best for this review since it's a potentially
>>> much
>>> more viral change as it adds another smart pointer type.
>>>
>>> Dave, you're on it cause you're the one I talked to about this the most.
>>> :)
>>>
>>> Though it shares the same name as PassOwnPtr in WebKit, the is a
>>> "cleanroom"
>>> implementation forked off of the scoped_ptr implementation.
>>>
>>> Let me know what you think.
>>>
>>> An alternate approach is to add a limited "base::Passed()" type similar
>>> to
>>> base::Owned() here
http://codereview.chromium.**org/8209001/<http://codereview.chromium.org/8209...
can only work
>>> with base::Bind().
>>>
>>> Let me know what method you prefer.
>>>
>>> Description:
>>> Add a PassOwnPtr into base/memory.
>>>
>>> This can be used in conjunction with base::Bind() to pass ownership of
>>> data into
>>> a bound Callback.
>>>
>>> The class itself has no thread safety.  It requires external
>>> synchronization.
>>>
>>> BUG=96118
>>> TEST=new unittests.
>>>
>>>
>>> Please review this at
http://codereview.chromium.**org/8224026/<http://codereview.chromium.org/8224...
>>>
>>> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>>>
>>> Affected files:
>>>  M base/base.gyp
>>>  M base/base.gypi
>>>  M base/bind_unittest.cc
>>>  A base/memory/pass_own_ptr.h
>>>  A base/memory/pass_own_ptr_**unittest.cc
>>>
>>>
>>>
>>
>

[willchan no longer on Chromium, 2011-10-11 05:52:42]

On Mon, Oct 10, 2011 at 10:41 PM, Darin Fisher <darin@chromium.org> wrote:

> Hmm...
>
> base::Owned() makes a lot of sense, although I might consider naming it
> Retained() instead.  It is the opposite of Unretained().  Retained() could
> work with something that is either RefCounted{ThreadSafe} or a raw pointer.
>  If it is a raw pointer, then its lifetime would be bound to the lifetime of
> the callback.
>

I'm worried that this gets complicated, because it's not clear what it means
when you don't use base::Retained() or base::Unretained().


>
> The base::PassOwn() idea is interesting, but since it is coupled with a
> MaybeCapturePtr<T>, it ends up seeming a lot like PassOwnPtr.  Am I wrong
> about that?
>
> By the way, you might find base/platform_file.h interesting.  It has a
> PassPlatformFile class.  It works somewhat similarly.
>

I think PassPlatformFile is pretty similar to MaybeCapturePtr (I hate that
name btw). I think the major difference between MaybeCapturePtr<T> and
PassOwnPtr<T> is the use of a explicit memory transfer method. I think I
prefer the explicit call.


> On one hand, I'm a bit surprised we have gone this far without needing
> PassOwnPtr.  It does make some things cleaner in WebKit.  But, it isn't
> without its pitfalls.  How far would we get with something like Retained()?
>  Are there some real transfer-of-ownership use cases you are trying to
> address that requires PassOwnPtr?
>

Yes. A common case is loading data from the FILE thread and then passing it
to another thread for use. If it's a large chunk of data, then copying it is
inefficient and you want the transfer of ownership.


>
> -Darin
>
>
>
> On Mon, Oct 10, 2011 at 10:09 PM, William Chan (陈智昌) <
> willchan@chromium.org> wrote:
>
>> Darin, have you kept up to date with the other threads wrt base::Owned()
>> and everything? In case you want to refresh on the use cases, they're at
>> http://code.google.com/p/chromium/issues/detail?id=96118. I am in favor
>> of a base::Owned() that makes the Callback take ownership of the memory, and
>> a base::Pass() that transfers ownership to the Callback, until it invokes
>> the function pointer, at which point the memory is transferred to the
>> receiving function. I am torn about whether or not that receiving function's
>> prototype should have a raw pointer for the transferred memory, or a
>> PassOwnPtr.
>>
>> What are your thoughts here? I don't have enough experience with the
>> downsides of PassOwnPtr since all codebases I've worked with extensively
>> before have outlawed auto_ptr type stuff. If we don't use PassOwnPtr, I feel
>> like a raw pointer is not obvious enough that memory transfer is occurring.
>> Anyway, I defer to you, but I guess I'm now leaning against PassOwnPtr.
>>
>> On Mon, Oct 10, 2011 at 10:01 PM, Darin Fisher <darin@chromium.org>wrote:
>>
>>> There are some gotchas with WebKit's PassOwnPtr that may be worth
>>> reviewing.  Dave Levin may have some gems to share.
>>>
>>> The one I can think of goes like this:
>>>
>>> class Foo {
>>>  private:
>>>   scoped_ptr<X> blah_;
>>>  public:
>>>   void DoStuff(const PassOwnPtr<X>& param) {
>>>     blah_ = param;
>>>     UseParam(param);  // Oops, param is now null because assignment to
>>> blah_ stole the pointer!
>>>   }
>>> };
>>>
>>> This is a surprisingly common error.  Often, there is a lot of distance
>>> between the blah_ assignment and the UseParam call, and the UseParam call
>>> probably was written before the blah_ assignment.
>>>
>>> The key annoyance here is that the error is only really detectable at
>>> run-time :-(
>>>
>>> I'm not citing this to say that we shouldn't have PassOwnPtr.  I'm just
>>> wondering....
>>>
>>> -Darin
>>>
>>>
>>> On Mon, Oct 10, 2011 at 7:51 PM, <ajwong@chromium.org> wrote:
>>>
>>>> Reviewers: darin, levin,
>>>>
>>>> Message:
>>>> Darin, I think you might be best for this review since it's a
>>>> potentially much
>>>> more viral change as it adds another smart pointer type.
>>>>
>>>> Dave, you're on it cause you're the one I talked to about this the most.
>>>> :)
>>>>
>>>> Though it shares the same name as PassOwnPtr in WebKit, the is a
>>>> "cleanroom"
>>>> implementation forked off of the scoped_ptr implementation.
>>>>
>>>> Let me know what you think.
>>>>
>>>> An alternate approach is to add a limited "base::Passed()" type similar
>>>> to
>>>> base::Owned() here
http://codereview.chromium.**org/8209001/<http://codereview.chromium.org/8209...
can only work
>>>> with base::Bind().
>>>>
>>>> Let me know what method you prefer.
>>>>
>>>> Description:
>>>> Add a PassOwnPtr into base/memory.
>>>>
>>>> This can be used in conjunction with base::Bind() to pass ownership of
>>>> data into
>>>> a bound Callback.
>>>>
>>>> The class itself has no thread safety.  It requires external
>>>> synchronization.
>>>>
>>>> BUG=96118
>>>> TEST=new unittests.
>>>>
>>>>
>>>> Please review this at
http://codereview.chromium.**org/8224026/<http://codereview.chromium.org/8224...
>>>>
>>>> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>>>>
>>>> Affected files:
>>>>  M base/base.gyp
>>>>  M base/base.gypi
>>>>  M base/bind_unittest.cc
>>>>  A base/memory/pass_own_ptr.h
>>>>  A base/memory/pass_own_ptr_**unittest.cc
>>>>
>>>>
>>>>
>>>
>>
>

[awong, 2011-10-11 06:34:46]

On Oct 10, 2011 10:52 PM, "William Chan (陈智昌)" <willchan@chromium.org>
wrote:
>
> On Mon, Oct 10, 2011 at 10:41 PM, Darin Fisher <darin@chromium.org> wrote:
>>
>> Hmm...
>>
>> base::Owned() makes a lot of sense, although I might consider naming it
Retained() instead.  It is the opposite of Unretained().  Retained() could
work with something that is either RefCounted{ThreadSafe} or a raw pointer.
 If it is a raw pointer, then its lifetime would be bound to the lifetime of
the callback.
>
>
> I'm worried that this gets complicated, because it's not clear what it
means when you don't use base::Retained() or base::Unretained().
>
Also,  currently the indication to ref count in arguments is to pass a
scoped_refptr. This would case 2 mechanisms for expressing refcounts.

The parallel would be to just bind a scoped_ptr, but that does not work
since those aren't copyable.

>>
>>
>> The base::PassOwn() idea is interesting, but since it is coupled with a
MaybeCapturePtr<T>, it ends up seeming a lot like PassOwnPtr.  Am I wrong
about that?
>>
>> By the way, you might find base/platform_file.h interesting.  It has a
PassPlatformFile class.  It works somewhat similarly.
>
>
> I think PassPlatformFile is pretty similar to MaybeCapturePtr (I hate that
name btw). I think the major difference between MaybeCapturePtr<T> and
PassOwnPtr<T> is the use of a explicit memory transfer method. I think I
prefer the explicit call

Yes...the implicit destructive copy makes me nervous.  However the
MaybeCapturePtr is complex and aesthetically offensive.

Thinking completely out of band, what about just doing some sort of
templated object holder class which is effectively a scoped_ptr but named
nicer?  I mean, if we only had Owned, we could bind a dynamically allocated
scoped ptr and have everything work right and be explicit.  It's just ugly
looking to see a scoped_ptr*.

>
>>
>> On one hand, I'm a bit surprised we have gone this far without needing
PassOwnPtr.  It does make some things cleaner in WebKit.  But, it isn't
without its pitfalls.  How far would we get with something like Retained()?
 Are there some real transfer-of-ownership use cases you are trying to
address that requires PassOwnPtr?
>
>
> Yes. A common case is loading data from the FILE thread and then passing
it to another thread for use. If it's a large chunk of data, then copying it
is inefficient and you want the transfer of ownership.
>
>>
>>
>> -Darin
>>
>>
>>
>> On Mon, Oct 10, 2011 at 10:09 PM, William Chan (陈智昌) <
willchan@chromium.org> wrote:
>>>
>>> Darin, have you kept up to date with the other threads wrt base::Owned()
and everything? In case you want to refresh on the use cases, they're at
http://code.google.com/p/chromium/issues/detail?id=96118. I am in favor of a
base::Owned() that makes the Callback take ownership of the memory, and a
base::Pass() that transfers ownership to the Callback, until it invokes the
function pointer, at which point the memory is transferred to the receiving
function. I am torn about whether or not that receiving function's prototype
should have a raw pointer for the transferred memory, or a PassOwnPtr.
>>>
>>> What are your thoughts here? I don't have enough experience with the
downsides of PassOwnPtr since all codebases I've worked with extensively
before have outlawed auto_ptr type stuff. If we don't use PassOwnPtr, I feel
like a raw pointer is not obvious enough that memory transfer is occurring.
Anyway, I defer to you, but I guess I'm now leaning against PassOwnPtr.
>>>
>>> On Mon, Oct 10, 2011 at 10:01 PM, Darin Fisher <darin@chromium.org>
wrote:
>>>>
>>>> There are some gotchas with WebKit's PassOwnPtr that may be worth
reviewing.  Dave Levin may have some gems to share.
>>>>
>>>> The one I can think of goes like this:
>>>>
>>>> class Foo {
>>>>  private:
>>>>   scoped_ptr<X> blah_;
>>>>  public:
>>>>   void DoStuff(const PassOwnPtr<X>& param) {
>>>>     blah_ = param;
>>>>     UseParam(param);  // Oops, param is now null because assignment to
blah_ stole the pointer!
>>>>   }
>>>> };
>>>>
>>>> This is a surprisingly common error.  Often, there is a lot of distance
between the blah_ assignment and the UseParam call, and the UseParam call
probably was written before the blah_ assignment.
>>>>
>>>> The key annoyance here is that the error is only really detectable at
run-time :-(
>>>>
>>>> I'm not citing this to say that we shouldn't have PassOwnPtr.  I'm just
wondering....
>>>>
>>>> -Darin
>>>>
>>>>
>>>> On Mon, Oct 10, 2011 at 7:51 PM, <ajwong@chromium.org> wrote:
>>>>>
>>>>> Reviewers: darin, levin,
>>>>>
>>>>> Message:
>>>>> Darin, I think you might be best for this review since it's a
potentially much
>>>>> more viral change as it adds another smart pointer type.
>>>>>
>>>>> Dave, you're on it cause you're the one I talked to about this the
most. :)
>>>>>
>>>>> Though it shares the same name as PassOwnPtr in WebKit, the is a
"cleanroom"
>>>>> implementation forked off of the scoped_ptr implementation.
>>>>>
>>>>> Let me know what you think.
>>>>>
>>>>> An alternate approach is to add a limited "base::Passed()" type
similar to
>>>>> base::Owned() here http://codereview.chromium.org/8209001/ that can
only work
>>>>> with base::Bind().
>>>>>
>>>>> Let me know what method you prefer.
>>>>>
>>>>> Description:
>>>>> Add a PassOwnPtr into base/memory.
>>>>>
>>>>> This can be used in conjunction with base::Bind() to pass ownership of
data into
>>>>> a bound Callback.
>>>>>
>>>>> The class itself has no thread safety.  It requires external
synchronization.
>>>>>
>>>>> BUG=96118
>>>>> TEST=new unittests.
>>>>>
>>>>>
>>>>> Please review this at http://codereview.chromium.org/8224026/
>>>>>
>>>>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>>>>>
>>>>> Affected files:
>>>>>  M base/base.gyp
>>>>>  M base/base.gypi
>>>>>  M base/bind_unittest.cc
>>>>>  A base/memory/pass_own_ptr.h
>>>>>  A base/memory/pass_own_ptr_unittest.cc
>>>>>
>>>>>
>>>>
>>>
>>
>

[levin, 2011-10-11 22:21:08]

On 2011/10/11 05:01:10, darin wrote:
> There are some gotchas with WebKit's PassOwnPtr that may be worth reviewing.
>  Dave Levin may have some gems to share.
> 
> The one I can think of goes like this:
> 
> class Foo {
>  private:
>   scoped_ptr<X> blah_;
>  public:
>   void DoStuff(const PassOwnPtr<X>& param) {
>     blah_ = param;
>     UseParam(param);  // Oops, param is now null because assignment to blah_
> stole the pointer!
>   }
> };
> 
> This is a surprisingly common error.  Often, there is a lot of distance
> between the blah_ assignment and the UseParam call, and the UseParam call
> probably was written before the blah_ assignment.
> 
> The key annoyance here is that the error is only really detectable at
> run-time :-(

fwiw, I've never hit an issue or remember debugging an issue due to PassOwnPtr,
so I have no tidbits to offer but I understand the concern.

I think there are two ways to combat this:

1. Don't provide anything on pass_own_ptr except a copy constructor which takes
another pass_own_ptr. Don't even provide a way to auto cast to a ptr or extract
a pointer.  Force people to put it into a scoped_ptr to use it. (Side note: Why
isn't this pass_scoped_ptr? In WebKit the scoped_ptr equivalent is OwnPtr so
PassOwnPtr makes sense in that context.) 

2. Provide a method on scoped_ptr that explicitly consume a pass_scoped_ptr.

Now the code above looks like this:
   void DoStuff(const PassOwnPtr<X>& param) {
     blah_.Consume(param); // Or Retain or...
     UseParam(param);
   }

I think the error is more clear now.

Also it makes it impossible to do this:
   void DoStuff(const PassOwnPtr<X>& param) {
     blah_.Consume(param); // Or Retain or...
     param->DoStuff();
   }

btw, I wish there was something like PassOwnPtr in chromium because when I did
work in that code base, I felt dirty put in comments like: "The caller of this
method should take ownership of the returned value." "This method takes
ownership of |bar|." It felt fragile and something that would ideally be
documented by the code itself (and ideally in a way that makes sure you don't
have leaks and do the right thing).

PS If this was a big concern, I guess there could be a clang check to verify
that a pass_scoped_ptr item is only used once during its lifetime -- to hand off
to another pass_scoped_ptr or to get consumed by a scoped_ptr -- and not used as
a class/struct member.



> 
> I'm not citing this to say that we shouldn't have PassOwnPtr.  I'm just
> wondering....
> 
> -Darin
>

[darin (slow to review), 2011-10-11 22:53:03]

I prefer pass_scoped_ptr too.
-Darin

On Tue, Oct 11, 2011 at 3:21 PM, <levin@chromium.org> wrote:

> On 2011/10/11 05:01:10, darin wrote:
>
>> There are some gotchas with WebKit's PassOwnPtr that may be worth
>> reviewing.
>>  Dave Levin may have some gems to share.
>>
>
>  The one I can think of goes like this:
>>
>
>  class Foo {
>>  private:
>>   scoped_ptr<X> blah_;
>>  public:
>>   void DoStuff(const PassOwnPtr<X>& param) {
>>     blah_ = param;
>>     UseParam(param);  // Oops, param is now null because assignment to
>> blah_
>> stole the pointer!
>>   }
>> };
>>
>
>  This is a surprisingly common error.  Often, there is a lot of distance
>> between the blah_ assignment and the UseParam call, and the UseParam call
>> probably was written before the blah_ assignment.
>>
>
>  The key annoyance here is that the error is only really detectable at
>> run-time :-(
>>
>
> fwiw, I've never hit an issue or remember debugging an issue due to
> PassOwnPtr,
> so I have no tidbits to offer but I understand the concern.
>
> I think there are two ways to combat this:
>
> 1. Don't provide anything on pass_own_ptr except a copy constructor which
> takes
> another pass_own_ptr. Don't even provide a way to auto cast to a ptr or
> extract
> a pointer.  Force people to put it into a scoped_ptr to use it. (Side note:
> Why
> isn't this pass_scoped_ptr? In WebKit the scoped_ptr equivalent is OwnPtr
> so
> PassOwnPtr makes sense in that context.)
>
> 2. Provide a method on scoped_ptr that explicitly consume a
> pass_scoped_ptr.
>
> Now the code above looks like this:
>
>   void DoStuff(const PassOwnPtr<X>& param) {
>     blah_.Consume(param); // Or Retain or...
>     UseParam(param);
>   }
>
> I think the error is more clear now.
>
> Also it makes it impossible to do this:
>
>   void DoStuff(const PassOwnPtr<X>& param) {
>     blah_.Consume(param); // Or Retain or...
>     param->DoStuff();
>   }
>
> btw, I wish there was something like PassOwnPtr in chromium because when I
> did
> work in that code base, I felt dirty put in comments like: "The caller of
> this
> method should take ownership of the returned value." "This method takes
> ownership of |bar|." It felt fragile and something that would ideally be
> documented by the code itself (and ideally in a way that makes sure you
> don't
> have leaks and do the right thing).
>
> PS If this was a big concern, I guess there could be a clang check to
> verify
> that a pass_scoped_ptr item is only used once during its lifetime -- to
> hand off
> to another pass_scoped_ptr or to get consumed by a scoped_ptr -- and not
> used as
> a class/struct member.
>
>
>
>
>
>  I'm not citing this to say that we shouldn't have PassOwnPtr.  I'm just
>> wondering....
>>
>
>  -Darin
>>
>
>
>
http://codereview.chromium.**org/8224026/<http://codereview.chromium.org/8224...
>

[willchan no longer on Chromium, 2011-10-12 00:41:42]

pass_scoped_ptr == MaybeCapturePtr, right? I'm fine with that, although I'd
want to change the name.

Btw, I'm not sure if we want to advocate any const SmartPtr<X>& because
people found it confusing before, as discussed on chromium-dev awhile back
(I disagree, but I guess most people prefer not using smart pointers with
const references). So if we add a new smart pointer, try not to use one that
would require this pattern.

On Tue, Oct 11, 2011 at 3:52 PM, Darin Fisher <darin@chromium.org> wrote:

> I prefer pass_scoped_ptr too.
> -Darin
>
>
> On Tue, Oct 11, 2011 at 3:21 PM, <levin@chromium.org> wrote:
>
>> On 2011/10/11 05:01:10, darin wrote:
>>
>>> There are some gotchas with WebKit's PassOwnPtr that may be worth
>>> reviewing.
>>>  Dave Levin may have some gems to share.
>>>
>>
>>  The one I can think of goes like this:
>>>
>>
>>  class Foo {
>>>  private:
>>>   scoped_ptr<X> blah_;
>>>  public:
>>>   void DoStuff(const PassOwnPtr<X>& param) {
>>>     blah_ = param;
>>>     UseParam(param);  // Oops, param is now null because assignment to
>>> blah_
>>> stole the pointer!
>>>   }
>>> };
>>>
>>
>>  This is a surprisingly common error.  Often, there is a lot of distance
>>> between the blah_ assignment and the UseParam call, and the UseParam call
>>> probably was written before the blah_ assignment.
>>>
>>
>>  The key annoyance here is that the error is only really detectable at
>>> run-time :-(
>>>
>>
>> fwiw, I've never hit an issue or remember debugging an issue due to
>> PassOwnPtr,
>> so I have no tidbits to offer but I understand the concern.
>>
>> I think there are two ways to combat this:
>>
>> 1. Don't provide anything on pass_own_ptr except a copy constructor which
>> takes
>> another pass_own_ptr. Don't even provide a way to auto cast to a ptr or
>> extract
>> a pointer.  Force people to put it into a scoped_ptr to use it. (Side
>> note: Why
>> isn't this pass_scoped_ptr? In WebKit the scoped_ptr equivalent is OwnPtr
>> so
>> PassOwnPtr makes sense in that context.)
>>
>> 2. Provide a method on scoped_ptr that explicitly consume a
>> pass_scoped_ptr.
>>
>> Now the code above looks like this:
>>
>>   void DoStuff(const PassOwnPtr<X>& param) {
>>     blah_.Consume(param); // Or Retain or...
>>     UseParam(param);
>>   }
>>
>> I think the error is more clear now.
>>
>> Also it makes it impossible to do this:
>>
>>   void DoStuff(const PassOwnPtr<X>& param) {
>>     blah_.Consume(param); // Or Retain or...
>>     param->DoStuff();
>>   }
>>
>> btw, I wish there was something like PassOwnPtr in chromium because when I
>> did
>> work in that code base, I felt dirty put in comments like: "The caller of
>> this
>> method should take ownership of the returned value." "This method takes
>> ownership of |bar|." It felt fragile and something that would ideally be
>> documented by the code itself (and ideally in a way that makes sure you
>> don't
>> have leaks and do the right thing).
>>
>> PS If this was a big concern, I guess there could be a clang check to
>> verify
>> that a pass_scoped_ptr item is only used once during its lifetime -- to
>> hand off
>> to another pass_scoped_ptr or to get consumed by a scoped_ptr -- and not
>> used as
>> a class/struct member.
>>
>>
>>
>>
>>
>>  I'm not citing this to say that we shouldn't have PassOwnPtr.  I'm just
>>> wondering....
>>>
>>
>>  -Darin
>>>
>>
>>
>>
http://codereview.chromium.**org/8224026/<http://codereview.chromium.org/8224...
>>
>
>

[awong, 2011-10-12 03:05:07]

Dave and I white boarded a bit today on passownptr.  I think a lot of
concern is that there is destructive copy and smart pointer semantics.  We
were thinking of removing all of that....will send a code proposal out in a
bit.  Basically it would he a dumb data carrier like refcounteddata but not
refcounted.

-A

(On phone)
On Oct 11, 2011 5:41 PM, "William Chan (陈智昌)" <willchan@chromium.org> wrote:

[darin (slow to review), 2011-10-12 05:04:33]

Right... OK

On Tue, Oct 11, 2011 at 10:01 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> linked_ptr is not threadsafe.
>
> Sent from my iNexus.
> On Oct 11, 2011 9:39 PM, "Darin Fisher" <darin@chromium.org> wrote:
>
>> OK, interesting.  Perhaps we should be using linked_ptr more / under the
>> hood?
>> -Darin
>>
>> On Tue, Oct 11, 2011 at 8:05 PM, Albert J. Wong (王重傑) <
>> ajwong@chromium.org> wrote:
>>
>>> Dave and I white boarded a bit today on passownptr.  I think a lot of
>>> concern is that there is destructive copy and smart pointer semantics.  We
>>> were thinking of removing all of that....will send a code proposal out in a
>>> bit.  Basically it would he a dumb data carrier like refcounteddata but not
>>> refcounted.
>>>
>>> -A
>>>
>>> (On phone)
>>> On Oct 11, 2011 5:41 PM, "William Chan (陈智昌)" <willchan@chromium.org>
>>> wrote:
>>>
>>
>>

[awong, 2011-10-12 22:35:36]

Okay...I have been defeated by the C++ type system.

I think the best we can do is what Dave suggested originally, which is to
remove all the functionality from PassOwnPtr besides the Release() call, and
the destructive copy constructor.

-Albert


On Tue, Oct 11, 2011 at 10:04 PM, Darin Fisher <darin@chromium.org> wrote:

> Right... OK
>
>
> On Tue, Oct 11, 2011 at 10:01 PM, William Chan (陈智昌) <
> willchan@chromium.org> wrote:
>
>> linked_ptr is not threadsafe.
>>
>> Sent from my iNexus.
>> On Oct 11, 2011 9:39 PM, "Darin Fisher" <darin@chromium.org> wrote:
>>
>>> OK, interesting.  Perhaps we should be using linked_ptr more / under the
>>> hood?
>>> -Darin
>>>
>>> On Tue, Oct 11, 2011 at 8:05 PM, Albert J. Wong (王重傑) <
>>> ajwong@chromium.org> wrote:
>>>
>>>> Dave and I white boarded a bit today on passownptr.  I think a lot of
>>>> concern is that there is destructive copy and smart pointer semantics.  We
>>>> were thinking of removing all of that....will send a code proposal out in a
>>>> bit.  Basically it would he a dumb data carrier like refcounteddata but not
>>>> refcounted.
>>>>
>>>> -A
>>>>
>>>> (On phone)
>>>> On Oct 11, 2011 5:41 PM, "William Chan (陈智昌)" <willchan@chromium.org>
>>>> wrote:
>>>>
>>>
>>>
>

[awong, 2011-10-12 23:47:03]

PTAL.

The summary of what Dave and I were attempting, but ultimately failed at doing
was to create a PassOwnPtr that did not have a copy constructor.  Instead, to
"pass" it, you would need to do

PassOwnPtr<T> f(other_ptr.Pass());

other_ptr.Pass() would return a SpecialType<T> that PassOwnPtr would have a
constructor for.

The API would guarantee that only other_ptr.Pass() could introduce
SpecialType<T>, and only PassOwnPtr<T>(const SpecialType<T> &) could consume
this type.

The destructive copy logic would be stored in SpecialType<T>, which would behave
a whole lot like auto-ptr. However, no one would be able to declare a
SpecialType<T> which avoids the bad use cases that have been talked about.  All
users of PassOwnPtr<> would have to call "Pass()" or "Release()" explicitly
before they could read values out of the type.

Unfortunately, it turns out that itself PassOwnPtr<> must have copy semantics if
it is to be used as a parameter type or a return type.  Otherwise, using
temporaries will blow up.  Specifically this construct cannot work:

void Foo(PassOwnPtr<A> ptr);

// Result of Pass() is a temporary of SpecailType<T>.
// This is only allowed in C++11
Foo(PassOwnPtr<A>(ptr).Pass());

In C++98, the copy-constructor for PassOwnPtr *must* be visible. Ugh.

Reference: http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#391

[awong, 2011-10-13 00:42:03]

Actually, hold off.  I'm apparently about to eat crow.  Somehow this is now
working...

-Albert


On Wed, Oct 12, 2011 at 4:47 PM, <ajwong@chromium.org> wrote:

> PTAL.
>
> The summary of what Dave and I were attempting, but ultimately failed at
> doing
> was to create a PassOwnPtr that did not have a copy constructor.  Instead,
> to
> "pass" it, you would need to do
>
> PassOwnPtr<T> f(other_ptr.Pass());
>
> other_ptr.Pass() would return a SpecialType<T> that PassOwnPtr would have a
> constructor for.
>
> The API would guarantee that only other_ptr.Pass() could introduce
> SpecialType<T>, and only PassOwnPtr<T>(const SpecialType<T> &) could
> consume
> this type.
>
> The destructive copy logic would be stored in SpecialType<T>, which would
> behave
> a whole lot like auto-ptr. However, no one would be able to declare a
> SpecialType<T> which avoids the bad use cases that have been talked about.
>  All
> users of PassOwnPtr<> would have to call "Pass()" or "Release()" explicitly
> before they could read values out of the type.
>
> Unfortunately, it turns out that itself PassOwnPtr<> must have copy
> semantics if
> it is to be used as a parameter type or a return type.  Otherwise, using
> temporaries will blow up.  Specifically this construct cannot work:
>
> void Foo(PassOwnPtr<A> ptr);
>
> // Result of Pass() is a temporary of SpecailType<T>.
> // This is only allowed in C++11
> Foo(PassOwnPtr<A>(ptr).Pass())**;
>
> In C++98, the copy-constructor for PassOwnPtr *must* be visible. Ugh.
>
> Reference: http://www.open-std.org/jtc1/**sc22/wg21/docs/cwg_defects.**
> html#391<http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#391>
>
>
http://codereview.chromium.**org/8224026/<http://codereview.chromium.org/8224...
>

[awong, 2011-10-13 07:57:51]

Well what do ya know...I actually got it working.  There are 2 questions at the
that I really would like input on ASAP since I disappear for 2 weeks after
friday.

  1) Do we like the current semantics for PassOwnPtr
  2) What semantics do we want for Bind?

We can discuss better naming after it's decided whether or not the semantics
work.

============

Current semantics for PassOwnPtr<>:

If you have a PassOwnPtr<T>, you can only do 4 things:
   1) Destruct it which will delete the underlying pointer.
   2) Call Pass() it to transfer to another PassOwnPtr() 
   3) Call ToScopedPtr() to transfer ownership to a scoped_ptr<>
   4) Call is_valid() to see if the PassOwnPtr<> has had Pass() or ToScopedPtr()
called before (this is NOT a null check).

There are 2 types that show up:

  1) Parameters use PassOwnPtr<T>.
  2) Return Values use PassOwnPtr<T>::InterstitialType

To create a PassOwnPtr<T>:

  void Foo(PassOwnPtr<int> ptr);
  Foo(PassOwnPtr<int>(new int(3)).Pass());

It would be nice to have a MakePassOwnPtr so it just becomes:

  Foo(MakePassOwnPtr(new int(3)));

============

Current semantics for PassOwnPtr<> and Bind.

  1) Call Bind() with .Pass() to transfer ownership.
      Ex:  Closure cb = Bind(&Foo, PassOwnPtr<int>(new int(3).Pass());
  2) On first call to Run(), ownership is passed. Subsequent calls will CHECK().
      cb.Run();  // ptr in Foo will have the int* from above.
      cb.Run();  // Since the int* was transfered already, this CHECK() fails.

I would like to propose this semantic.  I have another prototype for it, but it
doesn't quite work yet.
  1) Call Bind() with .Pass() to transfer ownership.  Same as before.
      Ex:  Closure cb = Bind(&Foo, PassOwnPtr<int>(new int(3).Pass());
  2) On calling Run(), ownership is NOT transfered out of the callback until
Pass(), or ToScopedPtr is called.
  3) After ownership is transfered, subsequent calls will not immediate CHECK().
 Clients are then able to query is_valid().

Example:

void ProcessInt(PassOwnPtr<int> ptr, bool should_capture) {
  // bail early so we don't CHECK fail on ToScopedPtr() below if 
  // should_capture is true.
   if (!ptr.is_valid()) return;  

  if (should_capture) {
     scoped_ptr<int> result;
     ptr.ToScopedPtr(&result);
     cout << "Captured " << *ptr << ".\n";
  } else {
     cout << "Not Captured Yet.\n";
  }
}

Callback<void(bool)> cb =
   Bind(&ProcessInt, MakePassOwnPtr(new int(3)));

    cb.Run(false);  // "Not Captured Yet."
    cb.Run(false);  // "Not Captured Yet."
    cb.Run(true);  // "Captured 3."   The int* is now deleted.
    cb.Run(true);  // Nothing. Early return.
    cb.Run(false);  // Nothing. Early return.

This makes it so there's no automagic ownership transferal.

Thoughts?

-Albert


P.S.  If you're curious what was wrong previously,  it turns out the temporary
issue I was hitting only seems to blowup when calling through a function pointer
which is what "InvokerN::DoInvoke" does.

Since we know what that exact usage does, we can safely friend the InvokerN<>
template from PassOwnPtr and provide a private destructive copy constructor for
PassOwnPtr. Then things actually work.  I'll make a point tomorrow to check with
some language experts if there's a way to avoid even this friending.



On 2011/10/13 00:42:03, awong wrote:
> Actually, hold off.  I'm apparently about to eat crow.  Somehow this is now
> working...
> 
> -Albert
> 
> 
> On Wed, Oct 12, 2011 at 4:47 PM, <mailto:ajwong@chromium.org> wrote:
> 
> > PTAL.
> >
> > The summary of what Dave and I were attempting, but ultimately failed at
> > doing
> > was to create a PassOwnPtr that did not have a copy constructor.  Instead,
> > to
> > "pass" it, you would need to do
> >
> > PassOwnPtr<T> f(other_ptr.Pass());
> >
> > other_ptr.Pass() would return a SpecialType<T> that PassOwnPtr would have a
> > constructor for.
> >
> > The API would guarantee that only other_ptr.Pass() could introduce
> > SpecialType<T>, and only PassOwnPtr<T>(const SpecialType<T> &) could
> > consume
> > this type.
> >
> > The destructive copy logic would be stored in SpecialType<T>, which would
> > behave
> > a whole lot like auto-ptr. However, no one would be able to declare a
> > SpecialType<T> which avoids the bad use cases that have been talked about.
> >  All
> > users of PassOwnPtr<> would have to call "Pass()" or "Release()" explicitly
> > before they could read values out of the type.
> >
> > Unfortunately, it turns out that itself PassOwnPtr<> must have copy
> > semantics if
> > it is to be used as a parameter type or a return type.  Otherwise, using
> > temporaries will blow up.  Specifically this construct cannot work:
> >
> > void Foo(PassOwnPtr<A> ptr);
> >
> > // Result of Pass() is a temporary of SpecailType<T>.
> > // This is only allowed in C++11
> > Foo(PassOwnPtr<A>(ptr).Pass())**;
> >
> > In C++98, the copy-constructor for PassOwnPtr *must* be visible. Ugh.
> >
> > Reference: http://www.open-std.org/jtc1/**sc22/wg21/docs/cwg_defects.**
> > html#391<http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#391>
> >
> >
>
http://codereview.chromium.**org/8224026/%3Chttp://codereview.chromium.org/82...>
> >

[awong, 2011-11-01 20:40:17]

Reviving this now that I'm back from vacation.

Suggested reading order: pass_own_ptr.h, followed by unittests.

[levin, 2011-11-02 00:51:30]

Just a few comments and nothing major.

http://codereview.chromium.org/8224026/diff/24014/base/bind_unittest.cc
File base/bind_unittest.cc (right):

http://codereview.chromium.org/8224026/diff/24014/base/bind_unittest.cc#newco...
base/bind_unittest.cc:660: EXPECT_EQ(0, deletes);
Shouldn't these EXPECT calls be ASSERT?

If something goes wrong, you could be using invalid memory and causing the test
suite to crash.

http://codereview.chromium.org/8224026/diff/24014/base/bind_unittest.cc#newco...
base/bind_unittest.cc:694: EXPECT_EQ(0, deletes);
Why not "delete counter" here to avoid leaks in tests which make valgrind
unhappy?

http://codereview.chromium.org/8224026/diff/24014/base/memory/pass_scoped_ptr.h
File base/memory/pass_scoped_ptr.h (right):

http://codereview.chromium.org/8224026/diff/24014/base/memory/pass_scoped_ptr...
base/memory/pass_scoped_ptr.h:138: class PassScopedPtr {
It feels like it should be pass_scoped_ptr (since the we have scoped_ptr).

http://codereview.chromium.org/8224026/diff/24014/base/memory/pass_scoped_ptr...
base/memory/pass_scoped_ptr.h:206: CHECK(type_ != TYPE_INVALID) << "Pointer
already used.";
DCHECK?
(Are all Chromium check's debug only?)

http://codereview.chromium.org/8224026/diff/24014/base/memory/pass_scoped_ptr...
base/memory/pass_scoped_ptr.h:230: other.type_ = TYPE_INVALID;
Perhaps just hoist other.type_ = TYPE_INVALID out of the end of the if/else's to
the end of this method.

http://codereview.chromium.org/8224026/diff/24014/base/memory/pass_scoped_ptr...
File base/memory/pass_scoped_ptr_unittest.cc (right):

http://codereview.chromium.org/8224026/diff/24014/base/memory/pass_scoped_ptr...
base/memory/pass_scoped_ptr_unittest.cc:79: MakePassScopedPtr(logger));
If I call MakePassScopedPtr on a scoped_ptr, will it work?

Personally, I wish I could do that. Rather than MakePassScopedPtr(p.release()).
but I guess the other is ok for now and this could be added later if that
becomes a common pattern.

http://codereview.chromium.org/8224026/diff/24014/base/memory/pass_scoped_ptr...
base/memory/pass_scoped_ptr_unittest.cc:99: 
Why doesn't returntype need to be used/tested here? (Or should it never show up
in usage like this?)

[awong, 2011-11-11 00:21:58]

Hah.

Turns out I didn't actually manage to unittest the most obvious, basic, use
case.  And that the missing test found a serious API flaw.  Oops.

Turns out you have to use "const PassScopedPtr<T> o" everywhere otherwise C++
attempts to do a convert, then copy when passing by value.  This means you
basically have to ignore constness in the APIs.  Ugh.

http://codereview.chromium.org/8224026/diff/24014/base/bind_unittest.cc
File base/bind_unittest.cc (right):

http://codereview.chromium.org/8224026/diff/24014/base/bind_unittest.cc#newco...
base/bind_unittest.cc:660: EXPECT_EQ(0, deletes);
On 2011/11/02 00:51:30, levin wrote:
> Shouldn't these EXPECT calls be ASSERT?
> 
> If something goes wrong, you could be using invalid memory and causing the
test
> suite to crash.

Good catch. Fixed.

http://codereview.chromium.org/8224026/diff/24014/base/bind_unittest.cc#newco...
base/bind_unittest.cc:694: EXPECT_EQ(0, deletes);
On 2011/11/02 00:51:30, levin wrote:
> Why not "delete counter" here to avoid leaks in tests which make valgrind
> unhappy?

Don't need to. result owns it.  There's no way to leak with only calls to
PassScopedPtr<> :)

http://codereview.chromium.org/8224026/diff/24014/base/memory/pass_scoped_ptr.h
File base/memory/pass_scoped_ptr.h (right):

http://codereview.chromium.org/8224026/diff/24014/base/memory/pass_scoped_ptr...
base/memory/pass_scoped_ptr.h:138: class PassScopedPtr {
On 2011/11/02 00:51:30, levin wrote:
> It feels like it should be pass_scoped_ptr (since the we have scoped_ptr).

Yeah...but then we have WeakPtr<>.  scoped_ptr<> is lowercase_underscore cause
it matches the new C++ scoped_ptr<>.  scoped_refptr seems like a mistake.  I'm
not sure we should continue propagating the convention violation...

http://codereview.chromium.org/8224026/diff/24014/base/memory/pass_scoped_ptr...
base/memory/pass_scoped_ptr.h:206: CHECK(type_ != TYPE_INVALID) << "Pointer
already used.";
I actually prefer CHECK.  Otherwise, we're actually going to likely introduce
either a double-free or a use-after-free.  Better to CHECK() fail right away I
think.

On 2011/11/02 00:51:30, levin wrote:
> DCHECK?
> (Are all Chromium check's debug only?)

http://codereview.chromium.org/8224026/diff/24014/base/memory/pass_scoped_ptr...
base/memory/pass_scoped_ptr.h:230: other.type_ = TYPE_INVALID;
On 2011/11/02 00:51:30, levin wrote:
> Perhaps just hoist other.type_ = TYPE_INVALID out of the end of the if/else's
to
> the end of this method.

Good idea. Done.

http://codereview.chromium.org/8224026/diff/24014/base/memory/pass_scoped_ptr...
File base/memory/pass_scoped_ptr_unittest.cc (right):

http://codereview.chromium.org/8224026/diff/24014/base/memory/pass_scoped_ptr...
base/memory/pass_scoped_ptr_unittest.cc:79: MakePassScopedPtr(logger));
On 2011/11/02 00:51:30, levin wrote:
> If I call MakePassScopedPtr on a scoped_ptr, will it work?
> 
> Personally, I wish I could do that. Rather than
MakePassScopedPtr(p.release()).
> but I guess the other is ok for now and this could be added later if that
> becomes a common pattern.

I like that you have explicitly tell the scoped_ptr to release().  The other
solution would require using a reference, which is disallowed by Google style.

http://codereview.chromium.org/8224026/diff/24014/base/memory/pass_scoped_ptr...
base/memory/pass_scoped_ptr_unittest.cc:99: 
On 2011/11/02 00:51:30, levin wrote:
> Why doesn't returntype need to be used/tested here? (Or should it never show
up
> in usage like this?)

...cause I forgot to do it? :P

Added unittest.

[awong, 2011-11-11 16:20:50]

aaaand...I think I fixed it.  Using move semantic emulation from C++03.

The API morphed a little, but I think it is cleaner.

Functions can now return a PassOwnPtr<T>.  However, when using Bind(), you have
to call ptr.PassForBind() instead of ptr.Pass().

[awong, 2011-12-06 00:03:55]

This has been superseded by:
  http://codereview.chromium.org/8774032/

Closing this CL.


On 2011/11/11 16:20:50, awong wrote:
> aaaand...I think I fixed it.  Using move semantic emulation from C++03.
> 
> The API morphed a little, but I think it is cleaner.
> 
> Functions can now return a PassOwnPtr<T>.  However, when using Bind(), you
have
> to call ptr.PassForBind() instead of ptr.Pass().