Add Pass(), which implements move semantics, to scoped_ptr, scoped_array, and scoped_ptr_malloc.

Add Pass(), which implements move semantics, to scoped_ptr, scoped_array, and scoped_ptr_malloc.

This modification to the scopers implements the "moveable but not copyable" semantics that were introduced in C++11's unique_ptr<>.

With this, is now possible to use scopers as an argument type or a return type.  This signifies, in the type system, transfer of ownership into a function or out of a function respectively.  Calling, or returning such a function MUST use the temporary resulting from the scoper's Pass() function. You CANNOT just pass the scoper by copy as there is still no copy constructor or assignment operator; trying to do so will yield a compilation error.

This distinction makes it possible to avoid the implicit ownership transfer issues of auto_ptr, but still allow us to have compiler enforced ownership transfer.

Also adds a Passed() helper that allows using a scoper with Bind().

BUG=96118
TEST=new unittests

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

[awong, 2011-12-02 23:52:34]

This is the evolution of http://codereview.chromium.org/8224026.

But now, I'm not bothering to introduce a specific type.

I need to fix up the comments explaining usage on monday for PassScoped() with
Bind.  But here's the first pass.

[darin (slow to review), 2011-12-03 00:25:10]

This seems very cool to me.  +1

http://codereview.chromium.org/8774032/diff/5001/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/8774032/diff/5001/base/memory/scoped_ptr.h#new...
base/memory/scoped_ptr.h:62: // Notice that if you do not call Pass() when
retruning from CreateFoo(), or
retruning -> returning

[awong, 2011-12-06 00:02:07]

Ready for review.

@darin: FYI, I was unable to make Passed() take a scoper by reference.  I think
it's technically doable, but unless you feel strongly, I think it's just simpler
if we tell people to pass by pointer.  So

  scoped_ptr<Foo> f;
  Bind(&Func, Passed(&f));

Less than optimal, but maybe okay?

http://codereview.chromium.org/8774032/diff/5001/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/8774032/diff/5001/base/memory/scoped_ptr.h#new...
base/memory/scoped_ptr.h:62: // Notice that if you do not call Pass() when
retruning from CreateFoo(), or
On 2011/12/03 00:25:10, darin wrote:
> retruning -> returning

Done.

[darin (slow to review), 2011-12-06 00:52:00]

On Mon, Dec 5, 2011 at 4:02 PM, <ajwong@chromium.org> wrote:

> Ready for review.
>
> @darin: FYI, I was unable to make Passed() take a scoper by reference.  I
> think
> it's technically doable, but unless you feel strongly, I think it's just
> simpler
> if we tell people to pass by pointer.  So
>
>  scoped_ptr<Foo> f;
>  Bind(&Func, Passed(&f));
>
> Less than optimal, but maybe okay?


I see... Hmm, I was wondering though if we shouldn't encourage the
definition
of functions that return scoped_ptr<Foo>.  In WebKit, we get a ton of great
utility out of having functions return PassOwnPtr<Foo>.

I was imagining use cases like this:

scoped_ptr<Foo> SomeFunction();
Bind(&Func, Passed(SomeFunction());

If you had to pass by pointer, then this would turn into:

scoped_ptr<Foo> SomeFunction();
scoped_ptr<Foo> f = SomeFunction();
Bind(&Func, Passed(&f));

-Darin



>
>
>
> http://codereview.chromium.**org/8774032/diff/5001/base/**
>
memory/scoped_ptr.h<http://codereview.chromium.org/8774032/diff/5001/base/memory/scoped_ptr.h>
> File base/memory/scoped_ptr.h (right):
>
> http://codereview.chromium.**org/8774032/diff/5001/base/**
>
memory/scoped_ptr.h#newcode62<http://codereview.chromium.org/8774032/diff/5001/base/memory/scoped_ptr.h#newcode62>
> base/memory/scoped_ptr.h:62: // Notice that if you do not call Pass()
> when retruning from CreateFoo(), or
> On 2011/12/03 00:25:10, darin wrote:
>
>> retruning -> returning
>>
>
> Done.
>
>
http://codereview.chromium.**org/8774032/<http://codereview.chromium.org/8774...
>

[awong, 2011-12-06 00:57:23]

On Mon, Dec 5, 2011 at 4:51 PM, Darin Fisher <darin@chromium.org> wrote:

>
>
> On Mon, Dec 5, 2011 at 4:02 PM, <ajwong@chromium.org> wrote:
>
>> Ready for review.
>>
>> @darin: FYI, I was unable to make Passed() take a scoper by reference.  I
>> think
>> it's technically doable, but unless you feel strongly, I think it's just
>> simpler
>> if we tell people to pass by pointer.  So
>>
>>  scoped_ptr<Foo> f;
>>  Bind(&Func, Passed(&f));
>>
>> Less than optimal, but maybe okay?
>
>
> I see... Hmm, I was wondering though if we shouldn't encourage the
> definition
> of functions that return scoped_ptr<Foo>.  In WebKit, we get a ton of great
> utility out of having functions return PassOwnPtr<Foo>.
>
> I was imagining use cases like this:
>
> scoped_ptr<Foo> SomeFunction();
> Bind(&Func, Passed(SomeFunction());
>
> If you had to pass by pointer, then this would turn into:
>
> scoped_ptr<Foo> SomeFunction();
> scoped_ptr<Foo> f = SomeFunction();
> Bind(&Func, Passed(&f));
>

How about supporting exactly the following 3 syntaxes:

scoped_ptr<Foo> SomeFunction();
scoped_ptr<Foo> arg1(new Foo());
scoped_ptr<Foo> arg2(new Foo();

Bind(&Func, Passed(&arg1));               // statement 1
Bind(&Func, Passed(arg2.Pass()));       // statement 2
Bind(&Func, Passed(SomeFunction())); // statement 3

That lets you support functions easily, and lets you be lazy if you have a
declared variable.

Note that statement 2 and statement 3 are equivalent to the type system so
you have to allow both, or neither.

-Albert




>
>>
>>
>> http://codereview.chromium.**org/8774032/diff/5001/base/**
>>
memory/scoped_ptr.h<http://codereview.chromium.org/8774032/diff/5001/base/memory/scoped_ptr.h>
>> File base/memory/scoped_ptr.h (right):
>>
>> http://codereview.chromium.**org/8774032/diff/5001/base/**
>>
memory/scoped_ptr.h#newcode62<http://codereview.chromium.org/8774032/diff/5001/base/memory/scoped_ptr.h#newcode62>
>> base/memory/scoped_ptr.h:62: // Notice that if you do not call Pass()
>> when retruning from CreateFoo(), or
>> On 2011/12/03 00:25:10, darin wrote:
>>
>>> retruning -> returning
>>>
>>
>> Done.
>>
>>
http://codereview.chromium.**org/8774032/<http://codereview.chromium.org/8774...
>>
>
>

[willchan no longer on Chromium, 2011-12-07 16:24:30]

I have to go stare at the move emulation code a bit later today.

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h
File base/bind_helpers.h (right):

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h#newcode18
base/bind_helpers.h:18: // Passed() is for transfering moveable-but-not-copyable
types (eg. scoped_ptr)
s/transfering/transferring/
s/moveable/movable/

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h#newcode23
base/bind_helpers.h:23: // transfered ownership to the target function.
s/transfered/transferred/

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h#newcode120
base/bind_helpers.h:120: //   cb.Run();  // Foo() is now transfered to |arg| and
deleted.
s/transfered/transferred/

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h#newcode121
base/bind_helpers.h:121: //   cb.Run();  // This CHECK()s since Foo() already
been used once.
Is there actually a CHECK somewhere? Or does it SIGSEGV on a NULL deref?

OIC, nm, this is because the UnwrapTraits will invoke PassedWrapper::Pass()
which will CHECK on a second invocation. Sounds great.

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h#newcode135
base/bind_helpers.h:135: #include "base/memory/scoped_ptr.h"
This isn't necessary is it?

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h#newcode337
base/bind_helpers.h:337: // unnecessary copies, it is incompatible with
moveable-but-not-copyable
s/moveable/movable/

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h#newcode343
base/bind_helpers.h:343: // types and moveable-but-not-copyable types.  Thus we
introduce a wrapper type
s/moveable/movable/

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h#newcode344
base/bind_helpers.h:344: // that is copyable to transmit the get the correct
type information down into
transmit the get the correct type?

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h#newcode350
base/bind_helpers.h:350: explicit PassedWrapper(T scoper) : is_valid_(true),
scoper_(scoper.Pass()) {}
Does this work for anything with Pass() defined? Do we need more type safety
here?

http://codereview.chromium.org/8774032/diff/7011/base/callback_internal.h
File base/callback_internal.h (right):

http://codereview.chromium.org/8774032/diff/7011/base/callback_internal.h#new...
base/callback_internal.h:134: // Parameter traits for moveable-but-not-copyable
scopers.
s/moveable/movable/ everywhere in this file

http://codereview.chromium.org/8774032/diff/7011/base/callback_internal.h#new...
base/callback_internal.h:138: // transfered (aka. moved) to another instance of
the same type by calling a
transferred

http://codereview.chromium.org/8774032/diff/7011/base/callback_internal.h#new...
base/callback_internal.h:139: // helper function.  When used with Bind(), this
signfies transferrance of the
signifies
transferral

http://codereview.chromium.org/8774032/diff/7011/base/callback_internal.h#new...
base/callback_internal.h:153: typedef scoped_ptr<T> ForwardType;
Can we get away with forward declaring these scoper types and then typedef'ing
the forward declarations?

http://codereview.chromium.org/8774032/diff/7011/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/8774032/diff/7011/base/memory/scoped_ptr.h#new...
base/memory/scoped_ptr.h:41: // scoper's Pass() function or another funciton
that generates a temporary;
function

[darin (slow to review), 2011-12-07 17:42:18]

On Mon, Dec 5, 2011 at 4:56 PM, Albert J. Wong (王重傑) <ajwong@chromium.org>wrote:

> On Mon, Dec 5, 2011 at 4:51 PM, Darin Fisher <darin@chromium.org> wrote:
>
>>
>>
>> On Mon, Dec 5, 2011 at 4:02 PM, <ajwong@chromium.org> wrote:
>>
>>> Ready for review.
>>>
>>> @darin: FYI, I was unable to make Passed() take a scoper by reference.
>>>  I think
>>> it's technically doable, but unless you feel strongly, I think it's just
>>> simpler
>>> if we tell people to pass by pointer.  So
>>>
>>>  scoped_ptr<Foo> f;
>>>  Bind(&Func, Passed(&f));
>>>
>>> Less than optimal, but maybe okay?
>>
>>
>> I see... Hmm, I was wondering though if we shouldn't encourage the
>> definition
>> of functions that return scoped_ptr<Foo>.  In WebKit, we get a ton of
>> great
>> utility out of having functions return PassOwnPtr<Foo>.
>>
>> I was imagining use cases like this:
>>
>> scoped_ptr<Foo> SomeFunction();
>> Bind(&Func, Passed(SomeFunction());
>>
>> If you had to pass by pointer, then this would turn into:
>>
>> scoped_ptr<Foo> SomeFunction();
>> scoped_ptr<Foo> f = SomeFunction();
>> Bind(&Func, Passed(&f));
>>
>
> How about supporting exactly the following 3 syntaxes:
>
> scoped_ptr<Foo> SomeFunction();
> scoped_ptr<Foo> arg1(new Foo());
> scoped_ptr<Foo> arg2(new Foo();
>
> Bind(&Func, Passed(&arg1));               // statement 1
> Bind(&Func, Passed(arg2.Pass()));       // statement 2
> Bind(&Func, Passed(SomeFunction())); // statement 3
>
>
^^^ Looks good!



> That lets you support functions easily, and lets you be lazy if you have a
> declared variable.
>
> Note that statement 2 and statement 3 are equivalent to the type system so
> you have to allow both, or neither.
>
> -Albert
>
>
>
>
>>
>>>
>>>
>>> http://codereview.chromium.**org/8774032/diff/5001/base/**
>>>
memory/scoped_ptr.h<http://codereview.chromium.org/8774032/diff/5001/base/memory/scoped_ptr.h>
>>> File base/memory/scoped_ptr.h (right):
>>>
>>> http://codereview.chromium.**org/8774032/diff/5001/base/**
>>>
memory/scoped_ptr.h#newcode62<http://codereview.chromium.org/8774032/diff/5001/base/memory/scoped_ptr.h#newcode62>
>>> base/memory/scoped_ptr.h:62: // Notice that if you do not call Pass()
>>> when retruning from CreateFoo(), or
>>> On 2011/12/03 00:25:10, darin wrote:
>>>
>>>> retruning -> returning
>>>>
>>>
>>> Done.
>>>
>>>
http://codereview.chromium.**org/8774032/<http://codereview.chromium.org/8774...
>>>
>>
>>
>
>

[willchan no longer on Chromium, 2011-12-07 19:25:24]

Move emulation stuff seems fine too. It's unfortunate to introduce the macro,
but  I guess that's fine.

Overall, the main thing I'd like to do if possible is drop the extra
scoped_ptr.h includes that were added here. I *think* forward declarations
should work, but I could be wrong.

http://codereview.chromium.org/8774032/diff/7011/base/callback_internal.h
File base/callback_internal.h (right):

http://codereview.chromium.org/8774032/diff/7011/base/callback_internal.h#new...
base/callback_internal.h:178: // is impossible to implement a general
std::foward with C++11 due to a lack of
s/foward/forward/

http://codereview.chromium.org/8774032/diff/7011/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/8774032/diff/7011/base/memory/scoped_ptr.h#new...
base/memory/scoped_ptr.h:94: public: \
this line is unnecessary, right?

http://codereview.chromium.org/8774032/diff/7011/base/memory/scoped_ptr.h#new...
base/memory/scoped_ptr.h:194: // are implmenting move semantics.  See the
CPP_03_MOVE_EMULATION macro.
implementing

http://codereview.chromium.org/8774032/diff/7011/base/memory/scoped_ptr.h#new...
base/memory/scoped_ptr.h:425: // are implmenting move semantics.  See the
CPP_03_MOVE_EMULATION macro.
implementing

[awong, 2011-12-08 21:04:09]

All fixed.

For good measure, also ran it through:

  git diff --name-only origin/trunk | xargs  -n1 sh -c 'aspell --add-filter=ccpp
check "$@" < /dev/tty' aspell

Yay for xargs man pages.

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h
File base/bind_helpers.h (right):

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h#newcode18
base/bind_helpers.h:18: // Passed() is for transfering moveable-but-not-copyable
types (eg. scoped_ptr)
On 2011/12/07 16:24:34, willchan wrote:
> s/transfering/transferring/
> s/moveable/movable/

Done.

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h#newcode23
base/bind_helpers.h:23: // transfered ownership to the target function.
On 2011/12/07 16:24:34, willchan wrote:
> s/transfered/transferred/

Done.

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h#newcode120
base/bind_helpers.h:120: //   cb.Run();  // Foo() is now transfered to |arg| and
deleted.
On 2011/12/07 16:24:34, willchan wrote:
> s/transfered/transferred/

Done.

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h#newcode121
base/bind_helpers.h:121: //   cb.Run();  // This CHECK()s since Foo() already
been used once.
On 2011/12/07 16:24:34, willchan wrote:
> Is there actually a CHECK somewhere? Or does it SIGSEGV on a NULL deref?
> 
> OIC, nm, this is because the UnwrapTraits will invoke PassedWrapper::Pass()
> which will CHECK on a second invocation. Sounds great.

Yep.  There's a distinction between initializing Pass() with NULL and with
calling it twice. I will add comment.

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h#newcode135
base/bind_helpers.h:135: #include "base/memory/scoped_ptr.h"
On 2011/12/07 16:24:34, willchan wrote:
> This isn't necessary is it?

Not anymore. Removed.

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h#newcode337
base/bind_helpers.h:337: // unnecessary copies, it is incompatible with
moveable-but-not-copyable
On 2011/12/07 16:24:34, willchan wrote:
> s/moveable/movable/

Done.

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h#newcode343
base/bind_helpers.h:343: // types and moveable-but-not-copyable types.  Thus we
introduce a wrapper type
On 2011/12/07 16:24:34, willchan wrote:
> s/moveable/movable/

Done.

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h#newcode344
base/bind_helpers.h:344: // that is copyable to transmit the get the correct
type information down into
On 2011/12/07 16:24:34, willchan wrote:
> transmit the get the correct type?

Done.

http://codereview.chromium.org/8774032/diff/7011/base/bind_helpers.h#newcode350
base/bind_helpers.h:350: explicit PassedWrapper(T scoper) : is_valid_(true),
scoper_(scoper.Pass()) {}
On 2011/12/07 16:24:34, willchan wrote:
> Does this work for anything with Pass() defined? Do we need more type safety
> here?

It works for anything with Pass().  I think this is a good thing actually.

There is an ancillary check in CallbackTraits which serves at the whitelist. 
Added comment to look there.

http://codereview.chromium.org/8774032/diff/7011/base/callback_internal.h
File base/callback_internal.h (right):

http://codereview.chromium.org/8774032/diff/7011/base/callback_internal.h#new...
base/callback_internal.h:134: // Parameter traits for moveable-but-not-copyable
scopers.
On 2011/12/07 16:24:34, willchan wrote:
> s/moveable/movable/ everywhere in this file

Done.

http://codereview.chromium.org/8774032/diff/7011/base/callback_internal.h#new...
base/callback_internal.h:138: // transfered (aka. moved) to another instance of
the same type by calling a
On 2011/12/07 16:24:34, willchan wrote:
> transferred

Done.

http://codereview.chromium.org/8774032/diff/7011/base/callback_internal.h#new...
base/callback_internal.h:139: // helper function.  When used with Bind(), this
signfies transferrance of the
On 2011/12/07 16:24:34, willchan wrote:
> signifies
> transferral

Done.

http://codereview.chromium.org/8774032/diff/7011/base/callback_internal.h#new...
base/callback_internal.h:153: typedef scoped_ptr<T> ForwardType;
On 2011/12/07 16:24:34, willchan wrote:
> Can we get away with forward declaring these scoper types and then typedef'ing
> the forward declarations?

Tried it, but ran into an issue with scoped_ptr_malloc having a default value
for its second template argument.  You can't forward declare the type with a
default argument apparently.

The only way to fix it is to create a scoped_ptr_forward.h.  Doesn't seem worth
it to me.

http://codereview.chromium.org/8774032/diff/7011/base/callback_internal.h#new...
base/callback_internal.h:178: // is impossible to implement a general
std::foward with C++11 due to a lack of
On 2011/12/07 19:25:24, willchan wrote:
> s/foward/forward/

Done.

http://codereview.chromium.org/8774032/diff/7011/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/8774032/diff/7011/base/memory/scoped_ptr.h#new...
base/memory/scoped_ptr.h:41: // scoper's Pass() function or another funciton
that generates a temporary;
On 2011/12/07 16:24:34, willchan wrote:
> function

Done.

http://codereview.chromium.org/8774032/diff/7011/base/memory/scoped_ptr.h#new...
base/memory/scoped_ptr.h:94: public: \
On 2011/12/07 19:25:24, willchan wrote:
> this line is unnecessary, right?

Fixed to make it necessary. :)

http://codereview.chromium.org/8774032/diff/7011/base/memory/scoped_ptr.h#new...
base/memory/scoped_ptr.h:194: // are implmenting move semantics.  See the
CPP_03_MOVE_EMULATION macro.
On 2011/12/07 19:25:24, willchan wrote:
> implementing

Done.

http://codereview.chromium.org/8774032/diff/7011/base/memory/scoped_ptr.h#new...
base/memory/scoped_ptr.h:425: // are implmenting move semantics.  See the
CPP_03_MOVE_EMULATION macro.
On 2011/12/07 19:25:24, willchan wrote:
> implementing

Done.

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

LGTM