Announcing scoped_ptr<>::Pass(). The latest in pointer ownership technology!

[Albert J. Wong (王重傑)]

If you never write C++ APIs that take or return ownership of a pointer from a function or a Callback, then you can stop reading.

Tired of writing careful ownership transfer comments on your API, only to have future maintainers miss them and introduce subtle leaks or use-after-free bugs?  Jealous of the Webkit folk and their PassOwnPtr?

Well, despair no longer!  r113922 adds the Pass() function to the 3 fundamental scopers: scoped_ptr<>, scoped_array<>, and scoped_ptr_malloc.

Pass() is an implementation of "movable but not copyable semantics" and allows you to use the scopers as a parameter or return type.  Using a scoper, instead of a raw pointer, as a parameter expresses the ownership transfer semantics in the type system making it very hard for users of your API to accidentally leak or incorrect retain a reference to an object.  Enough words though, let's see code.

With Pass(), you can take a raw pointer + comment API:

  // Takes ownership of |o|.  Don't you dare delete it underneath me.

  void Initialize(MyObject* o)

  // Caller owns the return value.  Please, pretty-please, remember to delete it.

  MyObject* Create();

and convert it into:

    // No comments needed.  The scoped_ptr<>s say it all!

   void Initialize(scoped_ptr<MyObject> o);

   scoped_ptr<MyObject> Create();

It's completely type safe.  But that's not all.  Because of the "movable but not copyable semantics", Pass() does not suffer from the accidental ownership transfer problems that auto_ptr<> has.  In particular, notice that this code will *not* compile.

   scoped_ptr<MyObject> o = Create();

   Initialize(o);  // BANG! COMPILE FAILURE. NO COPY CONSTRUCTOR.

The only way to call Initialize() is by calling it with an rvalue of the matching type. Pragmatically it means you must may only call it with the result of Pass() or with the result of a function that returns the right scoper.  Example:

   scoped_ptr<MyObject> o = Create();

   Initialize(o.Pass());

   Initialize(Create());

The same rule applies when returning from a function.

   scoped_ptr<MyObject> Create() {

     // OK. The cast create an rvalue.

     return scoped_ptr<MyObject>(new MyObject());

   }

   scoped_ptr<MyObject> PassThur(scoped_ptr<MyObject> o) {

      // If this was just "return o;" you get a compile error.

      return o.Pass();

   }

Okay...great, but what if what you really want to do is transfer ownership of an object (like a data buffer) into an asynchronous Callback?  Good news!  Bind() also supports ownership transfer via the Passed() wrapper! Example:

   scoped_ptr<MyObject> obj = Create();

   message_loop_proxy_->PostTask(

     FROM_HERE,

     base::Bind(&Initialize, Passed(&obj));

After this, obj.get()  will be NULL.  When Initialized() is run, it will receive ownership of the object.  And best of all, if the MessageLoopProxy is detached so Initialize() is never run, the objection will still be deleted for you!

For the careful, you'll notice that we passed a pointer to the obj.  This is just a piece of syntactic sugar.  You could have just as easily written the following:

   message_loop_proxy_->PostTask(

      FROM_HERE,

      base::Bind(&Initialize, Passed(o.Pass()));

or if you have the result of a function:

   message_loop_proxy_->PostTask(

      FROM_HERE,

      base::Bind(&Initialize, Passed(Create()));

Okay, that's it!  Question? Comments?

(special thanks to levin@, willchan@, darin@, and akalin@ for developing the idea and doing the reviews).

-Albert

FAQ

Q: Where did this idea come from?

It's cribbed from C++11's unique_ptr<> which is a beefed up scoped_ptr<> with movable but not copyable semantics.  They use an external "std::move()" function.  We implemented it with a Pass() method.

In C++11, they use "rvalue references" to implement this functionality.  This specific implementation is based off of a C++98 unique_ptr emulation written by Howard Hinnant: http://home.roadrunner.com/~hinnant/unique_ptr03.html

Q: Why not use PassOwnPtr?

It was considered, but in the end, this felt cleaner.  First, we avoid introducing a new type.  Second, scoped_ptr stays non-copyable which requires the API user to do something explicit to express ownership transfer.

Q: Can OwnPtr be modified to have similar semantics?

Yes!  The amount of code is actually fairly small.

Q: Why do we need Passed()?  Why can't Bind() just understand the result of Pass()?

The arguments to Bind() are const& to avoid taking extra copies.  Because of this, we can't tell if the original argument was const or not.  This is normally not a problem because we always copy the argument we are binding. However, with Pass(), we are now doing a destructive transfer and the const correctness bites us.

Q: Why not just take a normal reference in Bind()?

If you do that, then you can't bind the result of functions.

Q: This is crazy C++.  Are you sure it works?

Mostly sure.

Q: This is crazy C++.  Who's going to maintain it?

4 people reviewed it and understood it as needed, so I think it'll be okay.  It's also a moderately well known technique.

Q: What exactly does "moveable by not copyable mean" mean?

First off, you need to distinguish the ideas of an lvalue from an rvalue.  Loosely, an lvalue is any type that can be named (a variable, a parameter, etc.).  An rvalue is a temporary, or the result of a return function.

The observation is that for some types, the rvalues can be just considered transient carriers.  If you have a way to distinguish when a function is called with an rvalue as opposed to an lvalue, you can decide to just destructively take the state of the temporary instead of make a copy.  "Move semantics" mean that you make that distinction and invoke a different constructor that does the move instead of copy.  In C++11, this is done with a "move constructor" using an rvalue reference.

Moveable by not copyable means you *only* provide the move constructor. Thus, you can only transfer state from one instance to another if the original instance is an rvalue (function return, result of cast, etc), not a lvalue (a variable, a parameter, etc.).

Q: Is there any place that explains how this works?

I write a plus post with all the various resources here: https://plus.sandbox.google.com/u/0/109647692032961252090/posts/LiZxYj9bVcG

[Tommi]

Hey Albert,

Great to see improvements in these classes and great that there's now a specific way to declare ownership transfers.

I am assuming that this replaces these two possible ways of accomplishing the same thing:

TakeOwnership(my_scoped_ptr.release());

and

TakeOwnership(&my_scoped_ptr);

where:

void TakeOwnership(scoped_ptr<type>* p) { my_foo_.reset(p->release()); }

I've got one question and a question/feature request:

Q: How does the new approach compare in code size to the above approaches?

Question/Feature request (no good deed goes unpunished, right?):

One gripe I have with the scoped_* classes is that there is no way to transfer ownership of an object when it is passed back via an output

parameter.  I'm actually very surprised that this isn't supported as in my experience (which lies mostly on Windows), this is a very common

thing to do.  In fact, it's so common in Windows APIs that I included Receive() methods (we're discouraged from overriding operators) in

ScopedVariant, ScopedComPtr and ScopedBstr when I wrote them.  Without them, one would have to either use a temporary, or not use a

scoped class at all - i.e. having a Receive() is a necessity.

Is there a solution for the scoped_* classes that supports the below scenario?  Would adding a .receive() method to the scoped_ classes

with the accompanied checks for leaks etc be a bad idea or is there perhaps an even better (and more efficient) way of doing it?

int CreateFoo(Foo** ret);

(think of the return value being 0 for success or valuable info on why the operation failed when it does)

Cheers and thanks again for the improvement,

Tommi

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Jonathan Dixon]

This looks neat. Couple quick questions - 

- Should Pass() have WARN_UNUSED_RESULT annotation?

- Do you plan to merge it into the other ~4 copies of scope_ptr.h we have?

http://codesearch.google.com/#search/&exact_package=chromium&q=class%20scoped_ptr&type=cs

I'm passively wondering about the consequences of forking this from the 'upstream' source of scoped_ptr. While usage of it is hidden under Bind() it's one thing, as hand written code makes increasing use of it, it will gradually make the code less familiar for anyone used to other google3-style projects (with implications for anyone seeking c++ readability, for example), and impact on code portability 

--

[Albert J. Wong (王重傑)]

Hi Tommi,

Thanks for the comments.  Responses in line.

On Mon, Dec 12, 2011 at 4:43 AM, Tommi <to...@chromium.org> wrote:

Hey Albert,

Great to see improvements in these classes and great that there's now a specific way to declare ownership transfers.

I am assuming that this replaces these two possible ways of accomplishing the same thing:

TakeOwnership(my_scoped_ptr.release());

and

TakeOwnership(&my_scoped_ptr);

where:

void TakeOwnership(scoped_ptr<type>* p) { my_foo_.reset(p->release()); }

Yes.  The movable semantics should completely subsume this usage. Passing a scoped_ptr by pointer is relatively uncommon. A coarse grep shows 124 occurrences in the code base.

  $ git grep --cached "scoped_ptr<.*>\*" . | wc -l

  124

Part of the goal for Pass() was to reduce passing a scoped_ptr by reference or pointer in APIs.  This lets reviewers know easily that they need to look closer going on if they ever see a scoper passed by reference or pointer (notable exception is scoped_refptr).

If you think about it, ideally a scoper is a simple way of tying a dynamic object into a scope so it behaves like an auto.  Introducing semantics where you can pass the "scope" itself around is a little odd.

 

I've got one question and a question/feature request:

Q: How does the new approach compare in code size to the above approaches?

Good question...I just disassembled a couple of test functions using clang at -O0 and -O2.  The code was:

class Yay {};

extern void ScoperByPtr(scoped_ptr<Yay>* p) {  scoped_ptr<Yay> own(p->release()); };

extern void ScoperByMove(scoped_ptr<Yay> p) {};

scoped_ptr<Yay> p;

extern void UseByPtr() {  ScoperByPtr(&p); }

extern void UseByMove() { ScoperByMove(p.Pass()); }

I compiled each function individually (commented out the others) and the resulting object file sizes are as follows:

Func          -O0     -O2

UseByMove     13920   7928

UseByPtr      8616    7216

ScoperByMove  5344    2928

ScoperByPtr   9256    7312

Total Move:   19264   10856

Total Ptr:    17872   14528

The totals are a bit hard to read.

Looking at the dissassmbly, what's happened is one scoped_ptr construction/destruction has moved from inside the function to where the function is called.  The temporary "Rvalue" struct that has been introduced compiles out; it's just struct holding a reference so it's effectively just primitive pointer anyways.

So, the change is we have more cost per call rather than per function where the cost is one constructor and destruction of a scoped pointer (seems to be about 3 instructions in front and 4 instructions behind the call).  That's unfortunate.  I'm not sure how to evaluate what that effect is overall.

Question/Feature request (no good deed goes unpunished, right?):

One gripe I have with the scoped_* classes is that there is no way to transfer ownership of an object when it is passed back via an output

parameter.  I'm actually very surprised that this isn't supported as in my experience (which lies mostly on Windows), this is a very common

thing to do.  In fact, it's so common in Windows APIs that I included Receive() methods (we're discouraged from overriding operators) in

ScopedVariant, ScopedComPtr and ScopedBstr when I wrote them.  Without them, one would have to either use a temporary, or not use a

scoped class at all - i.e. having a Receive() is a necessity.

Is there a solution for the scoped_* classes that supports the below scenario?  Would adding a .receive() method to the scoped_ classes

with the accompanied checks for leaks etc be a bad idea or is there perhaps an even better (and more efficient) way of doing it?

int CreateFoo(Foo** ret);

(think of the return value being 0 for success or valuable info on why the operation failed when it does)

Hmm...to me, this really does feel like a situation where scoped_ptr<>* makes sense.  Would that solve it?

-Albert

[Albert J. Wong (王重傑)]

On Mon, Dec 12, 2011 at 5:31 AM, Jonathan Dixon <jo...@chromium.org> wrote:

This looks neat. Couple quick questions - 

- Should Pass() have WARN_UNUSED_RESULT annotation?

Sure.  It's not as critical as release() since Pass() without being used won't leak.  But it can't hurt.

 

- Do you plan to merge it into the other ~4 copies of scope_ptr.h we have?

http://codesearch.google.com/#search/&exact_package=chromium&q=class%20scoped_ptr&type=cs

I'm passively wondering about the consequences of forking this from the 'upstream' source of scoped_ptr. While usage of it is hidden under Bind() it's one thing, as hand written code makes increasing use of it, it will gradually make the code less familiar for anyone used to other google3-style projects (with implications for anyone seeking c++ readability, for example), and impact on code portability 

I hadn't planned on it.  I'll shoot off an e-mail to the google3 base people to see what they think.  Chromium code is somewhat divergent from Google3 already in idioms though (eg., scoped_refptr and anything relating to callbacks).  But you're right, this is a moderately large fork in behavior.

-Albert

[Peter Kasting]

This is super awesome.  I would love to see the same sort of initiative to replace existing ownership transfers with this machinery that we've had to upgrade code to use base::Bind().  I suspect it's harder to find all the cases where this could happen, though.

I'm also excited by your comment on the bug that once this appears to have stuck successfully, we can also add support for this to the other scoped_* and Scoped* objects.  I have an outstanding change that would really benefit from this :)

PK

[Achuith Bhandarkar]

This is great! Please correct me if I'm wrong, but with this change, can we now require that all dynamic allocations take place within the constructor of a scoped_* object? If we could get rid of all explicit deletes, that would be really nice.

Should http://www.chromium.org/developers/coding-style be updated to mention required use of scoped_ptr + Pass for all transfers of ownership?

I was wondering why we wouldn't instead use 

void Initialize(const scoped_ptr<A>& var);

to save a temporary + copy, but then I realized var needs to be mutable or you can't call Pass.

If we do create a list of files to update with the new Pass mechanism, chrome_resource_dispatcher_host_delegate.cc should be on it - it's quite difficult to follow all the transfers of ownership.

--

[Albert J. Wong (王重傑)]

On Mon, Dec 12, 2011 at 1:39 PM, Achuith Bhandarkar <ach...@chromium.org> wrote:

This is great! Please correct me if I'm wrong, but with this change, can we now require that all dynamic allocations take place within the constructor of a scoped_* object? If we could get rid of all explicit deletes, that would be really nice.

The way I think of it is that Pass() lets you express transfer of ownership from one function context to another.  As for initial capture of a raw pointer in a scoped_ptr, I think those semantics are still the same as before.

 

Should http://www.chromium.org/developers/coding-style be updated to mention required use of scoped_ptr + Pass for all transfers of ownership?

Before going that way, it'd be good to get a few concrete uses and see if we hit any serious API issues.  Joao just ran into 2 gotchas here http://codereview.chromium.org/8892034/.

  1) PostTaskAndReply(Bind(&Request, ptr.get()), Bind(&Reply, ptr.Pass()));  has undefined behavior. You might bind NULL to "Request" instead of the pointer value.

  2) GMock isn't compatible with movable-but-not-copyable types.

#2 concerns me cause I'm not sure there's a way to fix it at all and it effectively makes GMock unusable for APIs that use scoped_ptr as parameters/return values.

I was wondering why we wouldn't instead use 

void Initialize(const scoped_ptr<A>& var);

to save a temporary + copy, but then I realized var needs to be mutable or you can't call Pass.

Yep.  Also, if you take a const&, the type needs to have a visible copy constructor otherwise it's not compliant with C++98.

 

If we do create a list of files to update with the new Pass mechanism, chrome_resource_dispatcher_host_delegate.cc should be on it - it's quite difficult to follow all the transfers of ownership. 

On Mon, Dec 12, 2011 at 12:36 PM, Peter Kasting <pkas...@chromium.org> wrote:

This is super awesome.  I would love to see the same sort of initiative to replace existing ownership transfers with this machinery that we've had to upgrade code to use base::Bind().  I suspect it's harder to find all the cases where this could happen, though.

I'm not sure we need to scrub the whole codebase but if there's a few APIs or areas where ownership transfer is hard to track, migrating those might help a lot.

Not sure who would be the right person to lead such a thing though (I'm going on leave starting mid Jan so I can't pick that up).

-Albert

[Tommi]

Thanks Albert,

Hah, well, not the solution I was hoping for :)

The reason I think that scoped_ptr makes sense in the case of output pointers is because they make sense in the case of input pointers.

In both cases ownership is being transferred from one scope to the other.  Now there's "official" support for one direction but I'm wondering about the other. :)

[Albert J. Wong (王重傑)]

e in the case of output pointers is because they make sense in the case of input pointe

Hah, well, not the solution I was hoping for :)

The reason I think that scoped_ptr makes sensrs.

In both cases ownership is being transferred from one scope to the other.  Now there's "official" support for one direction but I'm wondering about the other. :)

I see where you're going....  I'm tempted to just say most APIs like int Create(Foo** ret) could be restructured to scoped_ptr<Foo> Create(int* error).

This avoids the whole class of type-system issues around a scoper as an output parameter. Making the error state the output parameter simplifies the API because no output parameter also also assigns ownership of a value.  Also, it makes it harder for people to create APIs that create many objects in one go which has a tendency to keep things narrower. But that's still avoiding the question. :)

I've mulled this for a few hours but can't think of anything better than scoped_ptr<>*.

To replace it, we would need to either introduce a new type that proxies the scoped_ptr<> and effectively be a reference, or we need to pass a pointer to the internal state like ScopedVariant::Receive(). Adding a new type seems very heavy weight for something like this.

As for passing a pointer to the internal raw-pointer, I think passing a scoped_ptr<>* is a little nicer.  By using scoped_ptr<>*, the API writer blocks the caller from using a raw pointer.

Since there isn't a legitimate reason for passing a scoped_ptr<>*, it could become idiomatic to use it as a safe "transfer-out" type.

Thoughts?

[Scott Hess]

I may be missing something about this discussion, but I have always
thought that:

RetValue MyFunction(scoped_ptr<X>* p);

Pretty nicely documents "I want you to return me a pointer, and I'm
required to handle ownership." The only downside to me would seem to
be the need to refer to (*p) in the implementation, rather than p, but
anything to resolve that would necessarily involve a certain amount of
hocus-pocus like a proxy object which only exists for purposes of
sugaring this case. But I think that passing a pointer to a
scoped_ptr<X> is easier to understand - it's hard to imagine anyone
doing Chromium development without having to know scoped_ptr<X>, but I
can easily imagine not having to deal with scoped_ptr<X>* very often
at all.

If it materially impacts things, you could always do:

RetValue MyFunction(scoped_ptr<X>* outp) {
scoped_ptr<X> p(new X(...));
// Working on p.
p.swap(*outp);
return GoodReturn;
}

-scott

On Mon, Dec 12, 2011 at 5:46 PM, Albert J. Wong (王重傑)

[Peter Kasting]

On Mon, Dec 12, 2011 at 6:46 PM, Scott Hess <sh...@chromium.org> wrote:

I may be missing something about this discussion, but I have always
thought that:

 RetValue MyFunction(scoped_ptr<X>* p);

Pretty nicely documents "I want you to return me a pointer, and I'm
required to handle ownership."

Yeah, but I agree with Albert that

scoped_ptr<X> MyFunction(/* optional outparam for detailed error code */);

...is better in almost every case.

PK

[Darin Fisher]

Delaying the swap until the point of "good return" is generally very good because that avoids setting out params in the case of an early return.

[Tommi]

To address your first instincts, where I've encountered this is not with Chromium code, but with third party code or platform APIs.

Chromium code returns the allocated object or NULL - which I suspect may be in part due to the fact that the scoped_ classes don't support the alternative.  This has pros and cons.  I guess the biggest pro would be that Chromium code is pretty consistent when it comes to this and a con would be that you will likely not have error information when doing post mortem debugging (e.g. why is this pointer NULL?).

Given that this is presumably most common where our code meets Windows APIs and we have scoped classes for those specific cases, I guess we can regard it being a solved problem and not involve the scoped_* classes in that discussion.

 

-Albert

[Matt Mueller]

2011/12/13 Tommi <to...@chromium.org>

scoped_ptr_malloc is used for various third party types (NSS being one example) and there are some times I've wished it had a Receive method.

-Albert

[Darin Fisher]

Yeah, this can be a very useful tool.  The Mozilla codebase had global functors, which addressed this problem.  If you grep their codebase, you can see getter_Copies and getter_AddRefs.  These take as argument the appropriate smart pointer and can be cast to a T**.

Perhaps we could layer a global Receive-style functor on top of our scoped* helper classes? 

Usage:

MyScopedClass foo;

obj->SomeFunction(Receive(&foo));

Strawman implementation:

template <typename T>

class  Receive {

 public:

  typedef T Scoper;

  typedef typename T::element_type element_type;

   Receive(Scoper* scoper) : scoper_(scoper), ptr_(NULL) {

  }

  ~Receive() {

    scoper_->reset(ptr_);

  }

  operator element_type**() {

    return &ptr_;

  }

 private:

  Scoper* scoper_;

  element_type* ptr_;

};

One could generalize this with a Traits class to abstract away the element_type typedef and reset function.

-Darin

[Tommi]

I think that's generally a good solution and it would work for any scoped class that has a reset method.

There is one thing that I like about the receive() method we have in the Win scoped classes

and that is the DCHECK that checks if the current pointer is NULL before passing in a reference,

thus pointing out leaks.

As is, the strawman implementation would silently delete any held object and replace it with a new

one.  While this would silently work, I worry that it could be misused and the code could hide that

there's an implicit 'free' of an unrelated object when passing in a reference to the scoper.

The programmer might also not be aware that he/she forgot to free the object at the right time/thread/etc.

I'm still more partial to adding a receive() method to the scoped classes, but if there is a good reason

for why we absolutely should not, then I think that this proposal is good but would like to add

DCHECK(scoper_->get() == NULL) (or DCHECK_EQ) to the destructor.

Tommi

[Tommi]

I've changed my mind.  I think that the Receive proposal goes more nicely with the

new Pass method than a receive method would.  So lgtm!  But can we add the

DCHECK to the destructor?

[Darin Fisher]

I'm not sure if that DCHECK always makes sense.  I can imagine a usage like this:

class A {

 public:

  ...

  void UpdateFoo();

 private:

  scoped_ptr<Foo> foo_;

};

void A::UpdateFoo() {

  low_level_system_->GetFoo(Receive(&foo_));

}

If it is required that foo_ be NULL before using Receive, then the above code would have to look like:

void A::UpdateFoo() {

  foo_.reset();

  low_level_system_->GetFoo(Receive(&foo_));

}

Isn't that a bit unfortunate?

-Darin

[Albert J. Wong (王重傑)]

Taking a step back to reframe the problem:

We're trying to solve 2 distinct use cases for passing ownership back from an function call:

 (1) API is in chromium.

 (2) We're calling a third-party API that uses a T**.

For (1), I think scoped_ptr<>* works great and we should probably encourage that in the majority of our code.

Looking at (2) more, I think Tommi's original reciever() that returns T** is probably the cleanest.  It'd be nice to name the function in a way that (eg., AdaptToHandleApi()) that discourages people from using it inside a pure chromium API.  But maybe it's not worth the extra typing?

The global receiver type type/function works, but I'm not sure I see a type safety or documentation benefit over embedding a AdaptToHandleApi() call.  On the down side, it adds another template symbol and will generate constructor/destructor calls for the temporary at the call site.

-Albert

[Darin Fisher]

On Wed, Dec 14, 2011 at 1:20 PM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

Taking a step back to reframe the problem:

We're trying to solve 2 distinct use cases for passing ownership back from an function call:

 (1) API is in chromium.

 (2) We're calling a third-party API that uses a T**.

For (1), I think scoped_ptr<>* works great and we should probably encourage that in the majority of our code.

Agreed.

 

Looking at (2) more, I think Tommi's original reciever() that returns T** is probably the cleanest.  It'd be nice to name the function in a way that (eg., AdaptToHandleApi()) that discourages people from using it inside a pure chromium API.  But maybe it's not worth the extra typing?

The global receiver type type/function works, but I'm not sure I see a type safety or documentation benefit over embedding a AdaptToHandleApi() call.  On the down side, it adds another template symbol and will generate constructor/destructor calls for the temporary at the call site.

-Albert

The main benefit to the global type is that it avoids us having to make changes to scoped_ptr.h or any of the other Scoped* classes.

[Peter Kasting]

On Wed, Dec 14, 2011 at 1:20 PM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

For (1), I think scoped_ptr<>* works great and we should probably encourage that in the majority of our code.

...if you need outparams.  I still want to wave my hands in the air and ask people to use direct-returns where possible for this kind of thing.

PK 

[Albert J. Wong (王重傑)]

On Wed, Dec 14, 2011 at 1:25 PM, Darin Fisher <da...@chromium.org> wrote:

On Wed, Dec 14, 2011 at 1:20 PM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

Taking a step back to reframe the problem:

We're trying to solve 2 distinct use cases for passing ownership back from an function call:

 (1) API is in chromium.

 (2) We're calling a third-party API that uses a T**.

For (1), I think scoped_ptr<>* works great and we should probably encourage that in the majority of our code.

Agreed.

 

Looking at (2) more, I think Tommi's original reciever() that returns T** is probably the cleanest.  It'd be nice to name the function in a way that (eg., AdaptToHandleApi()) that discourages people from using it inside a pure chromium API.  But maybe it's not worth the extra typing?

The global receiver type type/function works, but I'm not sure I see a type safety or documentation benefit over embedding a AdaptToHandleApi() call.  On the down side, it adds another template symbol and will generate constructor/destructor calls for the temporary at the call site.

-Albert

The main benefit to the global type is that it avoids us having to make changes to scoped_ptr.h or any of the other Scoped* classes.

ic.

There's 53 scoped*.h headers...

  $ git ls-tree -r HEAD --name-only | grep /scoped.*\.h | wc -l

  53

That's a lot, but still manageable to if really wanted to add a function everywhere.

How does that compare to the cost of the extra template symbol + constructor/destructor per call site?

-Albert

[Yuta Kitamura]

Hi Albert,

scoped_ptr<>::Pass() is great and I'm pretty happy to see it in Chromium.

I have one question: How do I transfer the ownership of a pointer as base class's pointer?

While I was writing some code using Pass(), I found that the following code doesn't compile:

class B {};

class D : public B {};

void f(scoped_ptr<B> b) {}

scoped_ptr<D> d(new D);

f(d.Pass());  // Error: conversion from 'scoped_ptr<D>' to non-scalar type 'scoped_ptr<B>' requested

For now, I work this around by replacing the last line with:

f(scoped_ptr<B>(d.release()));

... but I do not want to spell out the full type name of B (which is usually longer). Is there any other better/cleaner/recommended way to pass d to f()?

Thanks,

Yuta

--

[Jonathan Dixon]

On 27 December 2011 07:01, Yuta Kitamura <yu...@chromium.org> wrote:

Hi Albert,

scoped_ptr<>::Pass() is great and I'm pretty happy to see it in Chromium.

I have one question: How do I transfer the ownership of a pointer as base class's pointer?

I think it could be as simple as introducing a templated constructor on scoped_ptr's RValue to fix this?

  struct RValue { \

    template<typedef U>
    explicit RValue(U& obj) : obj_(obj) {} \
    scoper& obj_; \
  }; \

question would be, what other and more dangerous conversions would this then allow? Even in the example below, it likely be wrong if B does not have a virtual destructor, and if the ownership transfer happens transparently it would be easy to overlook this mistake. It feels akin to object slicing, although is not quite as bad as that!

Other thought is whether this woks against the c++11 'move emulation' design.

[Yuta Kitamura]

On Tue, Dec 27, 2011 at 5:51 PM, Jonathan Dixon <jo...@chromium.org> wrote:

On 27 December 2011 07:01, Yuta Kitamura <yu...@chromium.org> wrote:

Hi Albert,

scoped_ptr<>::Pass() is great and I'm pretty happy to see it in Chromium.

I have one question: How do I transfer the ownership of a pointer as base class's pointer?

I think it could be as simple as introducing a templated constructor on scoped_ptr's RValue to fix this?

  struct RValue { \

    template<typedef U>
    explicit RValue(U& obj) : obj_(obj) {} \
    scoper& obj_; \
  }; \

question would be, what other and more dangerous conversions would this then allow? Even in the example below, it likely be wrong if B does not have a virtual destructor, and if the ownership transfer happens transparently it would be easy to overlook this mistake. It feels akin to object slicing, although is not quite as bad as that!

My intention was that B and D have virtual destructor; actually there were virtual destructors in my original example but they slipped away while I was trying to make the code example as small as possible. Object slicing -- do not want!

[Albert J. Wong (王重傑)]

On Tue, Dec 27, 2011 at 1:22 AM, Yuta Kitamura <yu...@chromium.org> wrote:

On Tue, Dec 27, 2011 at 5:51 PM, Jonathan Dixon <jo...@chromium.org> wrote:

On 27 December 2011 07:01, Yuta Kitamura <yu...@chromium.org> wrote:

Hi Albert,

scoped_ptr<>::Pass() is great and I'm pretty happy to see it in Chromium.

I have one question: How do I transfer the ownership of a pointer as base class's pointer?

I think it could be as simple as introducing a templated constructor on scoped_ptr's RValue to fix this?

  struct RValue { \

    template<typedef U>
    explicit RValue(U& obj) : obj_(obj) {} \
    scoper& obj_; \
  }; \

question would be, what other and more dangerous conversions would this then allow? Even in the example below, it likely be wrong if B does not have a virtual destructor, and if the ownership transfer happens transparently it would be easy to overlook this mistake. It feels akin to object slicing, although is not quite as bad as that!

My intention was that B and D have virtual destructor; actually there were virtual destructors in my original example but they slipped away while I was trying to make the code example as small as possible. Object slicing -- do not want!

I think we can do this by adding a templated constructor and operator= into scoped_ptr<> itself.  Here's an example CL:

http://codereview.chromium.org/8968032

The new templated constructor and operator= still takes a scoped_ptr<U> by value so it maintains the move-only semantics. The assignability check is enforced as normal via pointer assignment in the initializer list or during the call to reset().

I had previously left this out cause I didn't think people wanted the functionality.

If no one object to the extra complexity, this CL should do the trick.

-Albert

[Yusuke Sato]

+1 for adding the templated functions. I ran into the same problem as yutak yesterday.

-Yusuke

[Jonathan Dixon]

-1 for adding copy constructor & assignment operator to scoped_ptr.

Mutating the copied-value is subtle, and diverges a good deal further from what the rest of the world defines a scoped_ptr to be:

"""

http://www.boost.org/doc/libs/1_48_0/libs/smart_ptr/scoped_ptr.htm

The scoped_ptr template is a simple solution for simple needs. It supplies a basic "resource acquisition is initialization" facility, without shared-ownership or transfer-of-ownership semantics. Both its name and enforcement of semantics (by being noncopyable) signal its intent to retain ownership solely within the current scope. Because it is noncopyable, it is safer than shared_ptr or std::auto_ptr for pointers which should not be copied.

"""

In particular, this looks like it would open up scoped_ptr to auto_ptr's gotcha #68 (unexpected behavior when used in STL container)

It feels kinda like we're trying to achieve unique_ptr, but in world without move c'tor will end up with a new variant of auto_ptr instead?

[Albert J. Wong (王重傑)]

On Tue, Jan 3, 2012 at 3:39 AM, Jonathan Dixon <jo...@chromium.org> wrote:

-1 for adding copy constructor & assignment operator to scoped_ptr.

Yeah...this make me pause initially too.  However, I don't believe this is actually a copy constructor or an assignment operator.

Since both the new template constructor, and the template assignment operator take the scoped_ptr<> by value, we preserve the non-copyable semantics. The constructor and operator= behave like any other function/method that is expressing ownership transfer with scoped_ptr. Thus, we are still move-only.

To test, grab the patch and attempt to assign or copy construct a scoped_ptr.  It shouldn't work unless you are coming via an rvalue (eg., as the result of a call to Pass() or as a return to a function).

What this CL allows is the following:

  class Parent{};

  class Child : public Parent {};

  scoped_ptr<Child> CreateChild();

  scoped_ptr<Parent> p = CreateChild();

  scoped_ptr<Child> c = CreateChild();

  scoped_ptr<Parent> p2_copy(c.Pass());

  scoped_ptr<Parent> p2_assign;

  p2_assign = c.Pass();

All this still does NOT work:

  scoped_ptr<Child> c = CreateChild();

  scoped_ptr<Parent> p2_no_pass(c);  // COMPILE ERROR on Copy Construct

  scoped_ptr<Parent> p2_assign_no_pass;

  p2_assign_no_pass = c;    // COMPILE ERROR on operator=

  p2_assign_no_pass = p;    // COMPILE ERROR on operator=

  void TakesParent(scoped_ptr<Parent> ptr);

  TakesParent(c.Pass());  // Sad...the CL I have only fixes assignment from a return, but not calling a function.

However, it's a fair point that we're basically turning our scoped_ptr<> into unique_ptr<>.  I'm don't think the extra convertible type is much of a divergence over the .Pass() support since it can only come into effect when you call Pass().  So if we keep .Pass(), it makes sense to add the templated constructor and operator=.

Does that sound reasonable?

-Albert

[William Chan (陈智昌)]

+akalin

I missed this discussion while on leave, but this is a significant flaw here. We should fix it. I'm going to read the issue in detail next week, but if you have strong opinions on the right way to fix this, I'd love to hear. Especially since Albert's on leave.

[Sergey Ulanov]

On Fri, Feb 3, 2012 at 3:16 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

+akalin

I missed this discussion while on leave, but this is a significant flaw here. We should fix it. I'm going to read the issue in detail next week, but if you have strong opinions on the right way to fix this, I'd love to hear. Especially since Albert's on leave.

I'm not sure which issue you are talking about here, but if it is about upcasting Child pointer to Parent, then I checked in a workaround for it in http://crrev.com/118958 . Basically the idea is to call PassAs<>() instead of Pass() when the pointer needs to be converted to a different type, e.g.:

  void TakesParent(scoped_ptr<Parent> ptr);

  scoped_ptr<Child> c = CreateChild();

  TakesParent(c.PassAs<Parent>()); 

Note that it is still typesafe. The example above will compile only when there is implicit conversion from Child* to Parent*.

[Fred Akalin]

I still have to catch up on this thread, but I didn't forget about it.
I'll try to get to this today.

On Fri, Feb 3, 2012 at 3:16 PM, William Chan (陈智昌)
<will...@chromium.org> wrote:

[Fred Akalin]

Okay, I reviewed the thread.  It sounds like it's fixed via PassAs()?  Am I missing anything?