proposal: allow std::shared_ptr and std::unique_ptr

[Matthew Dempsky]

I'd like to convert sandbox/linux to use std::shared_ptr and std::unique_ptr to remove some of its dependencies on base, and make it easier to reuse outside of Chromium.

The Google C++ style guide already allows and encourages use of these classes, and my impression is base's scoped_ptr is intended to be replaced by std::unique_ptr.

The situation for std::shared_ptr in Chromium seems less clear (intrusive vs non-intrusive ref counting), but at least within sandbox/linux the uses of scoped_refptr are all internal / implementation details that don't affect the end user API.

[Ryan Hamilton]

+1 for std::unique_ptr. QUIC and SPDY code is shared with an internal repository which uses std::unique_ptr. Being able to use std::unique_ptr in the Chromium code would result in fewer differences between the copies of the code.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF52%2BS4x0qQS1zvuJV8%3D5KqSG4R-wQV9DHhNS%3Dcz1SnQg9jj-A%40mail.gmail.com.

[Daniel Cheng]

I think we should consider std::shared_ptr separately.

I had a draft for std::unique_ptr, but since the thread already started, I'll just dump it here instead. std::unique_ptr is blocked on std::move(), since it's not as useful without it. A few things that I think are worth highlighting:

• converting between std::unique_ptr and scoped_ptr: in order to allow people to start using it incrementally, it's probably useful to have a way to convert back and forth between them. https://codereview.chromium.org/1432193002 is a proposal that would allow this.
• std::unique_ptr and base::Bind: base::Bind() and base::Passed() depends on the custom macros for move semantics, which add a Pass() member to movable types. One option is to use template metaprogramming magic to make base::Passed() magically use std::move() for types with move constructors/operators. Another is to add more magic to base::Bind() so it can implicitly convert std::unique_ptr to scoped_ptr.
  
• searching a container of unique_ptrs: since unique_ptr<T> is not comparable to T*, code like this won't work:
      std::set<std::unique_ptr<T>> s;
      std::unique_ptr<T> p(new T);
      T* raw_p = p.get();
      s.insert(std::move(p));
      auto it = s.find(raw_p); // error: unique_ptr<T> is not comparable to T*
  Instead, you'll have to write this:
      auto it = std::find_if(s.begin(), s.end(),
          [raw_p](const std::unique_ptr<T>& x) {
            return x.get() == raw_p;
          }); // O(n) instead of O(lg n)
• specializations of std::default_delete: currently, there are some specializations of DefaultDeleter so code doesn't have to specify a custom deleter when instantiating a scoped_ptr for that type [1]. User code isn't usually allowed to do random things in namespace std (the spec uses the word "undefined behavior" a lot here), but there's an exception for specializations in 17.6.4.2.1 and I believe this rule would apply to std::default_delete. It might be nice to have some explicit guidance around this.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJ_4DfR8G9z6kiVuq3BUHZGLD1fgXrrMTZgoAFNmBOPnVN3FGA%40mail.gmail.com.

[Daniel Cheng]

Oh, one other thing I forgot to mention: scoped_ptr<T> currently has make_scoped_ptr. For unique_ptr, there are several options:

1. Add a WrapUnique helper:
       auto p = WrapUnique(new RenderWidgetHostViewDelegateClientFactoryHelper(delegate, client));
   
   Why not call it make_unique_ptr? As it turns out, C++14 already has std::make_unique which has different semantics. This leads us to option...
   
   
2. Add a MakeUnique helper that does the same thing as C++14's std::make_unique.
       auto p = MakeUnique<RenderWidgetHostViewDelegateClientFactoryHelper>(delegate, client);
   
   
3. Do both (incidentally, this is what google3 did).
   
   
4. Do neither.

(This is assuming we allow auto to be used in this way.)

Daniel 

[Dana Jansens]

On Thu, Nov 12, 2015 at 2:08 PM, Daniel Cheng <dch...@chromium.org> wrote:

Oh, one other thing I forgot to mention: scoped_ptr<T> currently has make_scoped_ptr. For unique_ptr, there are several options:

1. Add a WrapUnique helper:
       auto p = WrapUnique(new RenderWidgetHostViewDelegateClientFactoryHelper(delegate, client));
   
   Why not call it make_unique_ptr? As it turns out, C++14 already has std::make_unique which has different semantics. This leads us to option...
   
   
2. Add a MakeUnique helper that does the same thing as C++14's std::make_unique.
       auto p = MakeUnique<RenderWidgetHostViewDelegateClientFactoryHelper>(delegate, client);
   

I'd prefer to be as close to the future-standard as possible. Just as any differences in scoped_ptr vs unique_ptr required us to write more complicated patches to convert things (in that case to avoid causing crashes or bugs), I'd also want things we do that will be replaced by C++14 to be as search-and-replaceable as possible. So I like base::MakeUnique better than base::WrapUnique.

The latter is of course closer to what we have with scoped_ptr, so it makes us write more patches up front now, instead of later.. but the incentive to do so to use std::unique_ptr is probably higher than the incentive to use std::make_unique, leaving us hopefully less likely to end up with both std::make_unique and base::something for an indefinitely long time.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKqFW91hRr1smPnDjJbxPF9YSFqFnrRy9C9as%2BL%2BavD9Hg%40mail.gmail.com.

[Nico Weber]

I too think it makes sense to handle unique_ptr and shared_ptr separately.

Allowing unique_ptr early makes sense to me (for quic if nothing else), and https://codereview.chromium.org/1432193002/ seems like a good way to get there.

WrapUnique() also sounds good to me. Is it feasible to mass-replace "make_scoped_ptr" to "wrap_scoped_ptr" to keep names similar? (/me shakes fist at standard for giving make_unique gratuitously different behavior than make_pair etc).

I suppose shared_ptr is morally equivalent to scoped_refptr, so we'd need to have some story around these two classes like we do for unique_ptr / scoped_ptr.

On Thu, Nov 12, 2015 at 2:08 PM, Daniel Cheng <dch...@chromium.org> wrote:

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKqFW91hRr1smPnDjJbxPF9YSFqFnrRy9C9as%2BL%2BavD9Hg%40mail.gmail.com.

[Jeffrey Yasskin]

On Thu, Nov 12, 2015 at 2:24 PM, Nico Weber <tha...@chromium.org> wrote:
> I too think it makes sense to handle unique_ptr and shared_ptr separately.

+1.

> I suppose shared_ptr is morally equivalent to scoped_refptr, so we'd need to
> have some story around these two classes like we do for unique_ptr /
> scoped_ptr.

There are some efficiency differences between shared_ptr and
scoped_refptr, which should be explored in the proposal to allow
shared_ptr. e.g. sizeof(shared_ptr)>sizeof(scoped_refptr), and
enable_shared_from_this also has overhead.

> https://groups.google.com/a/chromium.org/d/msgid/cxx/CAMGbLiG%2B2x5ruj%2B%2B7ti8gdddtzbdvqwKzwEoVLd6hf2kz7J%2BHA%40mail.gmail.com.

[Dana Jansens]

On Thu, Nov 12, 2015 at 2:28 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

On Thu, Nov 12, 2015 at 2:24 PM, Nico Weber <tha...@chromium.org> wrote:
> I too think it makes sense to handle unique_ptr and shared_ptr separately.

+1.

> I suppose shared_ptr is morally equivalent to scoped_refptr, so we'd need to
> have some story around these two classes like we do for unique_ptr /
> scoped_ptr.

There are some efficiency differences between shared_ptr and
scoped_refptr, which should be explored in the proposal to allow
shared_ptr. e.g. sizeof(shared_ptr)>sizeof(scoped_refptr), and
enable_shared_from_this also has overhead.

Yah. My general feelings are that:

1) I don't like using types in std:: that provide the same things as types in base::. Using different "standardish" types in different places feels awkward and confusing.

2) I prefer using types in std:: to having our own implementation of it in base:: when possible.

All that is to say, I don't feel excited about allowing std::shared_ptr while we also have scoped_refptr.

And I enthusiastically welcome data-driven proposals around replacing scoped_refptr with std::shared_ptr if this turns out to be reasonable.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CANh-dXntiTqm%2Baq6RkC9To5C4hrOwWAZqc-u1rscVjLBDz_gHg%40mail.gmail.com.

[Vladimir Levin]

On Thu, Nov 12, 2015 at 2:35 PM, 'Dana Jansens' via cxx <c...@chromium.org> wrote:

On Thu, Nov 12, 2015 at 2:28 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

On Thu, Nov 12, 2015 at 2:24 PM, Nico Weber <tha...@chromium.org> wrote:
> I too think it makes sense to handle unique_ptr and shared_ptr separately.

+1.

> I suppose shared_ptr is morally equivalent to scoped_refptr, so we'd need to
> have some story around these two classes like we do for unique_ptr /
> scoped_ptr.

There are some efficiency differences between shared_ptr and
scoped_refptr, which should be explored in the proposal to allow
shared_ptr. e.g. sizeof(shared_ptr)>sizeof(scoped_refptr), and
enable_shared_from_this also has overhead.

Yah. My general feelings are that:

1) I don't like using types in std:: that provide the same things as types in base::. Using different "standardish" types in different places feels awkward and confusing.

2) I prefer using types in std:: to having our own implementation of it in base:: when possible.

All that is to say, I don't feel excited about allowing std::shared_ptr while we also have scoped_refptr.

And I enthusiastically welcome data-driven proposals around replacing scoped_refptr with std::shared_ptr if this turns out to be reasonable.

I agree with this. I would say the biggest problem I have with scoped_refptr is that the class has to be declared as being ref counted, instead of the ref counting being external (as is the case in shared_ptr). However, I also realize that there are efficiency benefits to scoped_refptr that shared_ptr can't provide. So, we probably need data.

+1 to unique_ptr though. One thing to note is that shared_ptr can be constructed from a unique_ptr implicitly, so we have to be careful not to accidentally say "shared_ptr" somewhere.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaRQdfH0u-CeHsQSL9NJUuQerz_8-p%2Bwb-H405o0Y1%3D%2B_g%40mail.gmail.com.

[Matthew Dempsky]

On Thu, Nov 12, 2015 at 2:35 PM, Dana Jansens <dan...@google.com> wrote:

All that is to say, I don't feel excited about allowing std::shared_ptr while we also have scoped_refptr.

I agree we should have a recommended default.  I'm even fine with std::shared_ptr being discouraged in general.  I'd still like to be able to use it in parts of the tree that are intended to be reused outside of Chromium without depending on base.

E.g., I see in third_party/webrtc, there's a bunch of comments like "// TODO(solenberg): Change this to a shared_ptr once we can use C++11."  Is it okay for third_party/webrtc to use std::shared_ptr, or will they need to continue using their own rtc::scoped_refptr class?

Analogously, is it okay to use std::shared_ptr just as an implementation detail of sandbox/linux?  Would the answer change if we rename that directory to third_party/bauxite?  (Bauxite is already the name of the project to make sandbox/linux work outside of Chromium.)

[Nico Weber]

I think we should have a single guideline for stuff in src.git, else things will get super confusing.

[Matthew Dempsky]

On Thu, Nov 12, 2015 at 3:13 PM, Nico Weber <tha...@chromium.org> wrote:

I think we should have a single guideline for stuff in src.git, else things will get super confusing.

So we need to split sandbox/linux into bauxite.git to be able to use std::shared_ptr?

[Daniel Cheng]

Forking this since this discussion is largely about shared_ptr.

I think if sandbox/linux were to move into //third_party/bauxite, then it could probably do whatever it wants. But inside Chromium, shared_ptr feels largely redundant with scoped_refptr (not to mention we also have WTF::RefPtr).

Daniel  

[Nico Weber]

I agree with this. "If it's in chromium's repo, it follows Chromium's rules" is a simple rule that's easy to understand. I think it'd be nice for chromium-affine projects to do so too, but that's already not the case (webrtc, skia, …).

It also imposes some cost on deciding to do your own thing, which is probably a good incentive.

(Note that stuff outside of src isn't allowed to depend on base/; some bits of sandbox/linux do depend on base/ so I suppose these bits wouldn't be pulled out.)

[Brett Wilson]

On Thu, Nov 12, 2015 at 2:24 PM, Nico Weber <tha...@chromium.org> wrote:

I too think it makes sense to handle unique_ptr and shared_ptr separately.

Allowing unique_ptr early makes sense to me (for quic if nothing else), and https://codereview.chromium.org/1432193002/ seems like a good way to get there.

WrapUnique() also sounds good to me. Is it feasible to mass-replace "make_scoped_ptr" to "wrap_scoped_ptr" to keep names similar? (/me shakes fist at standard for giving make_unique gratuitously different behavior than make_pair etc).

I suppose shared_ptr is morally equivalent to scoped_refptr, so we'd need to have some story around these two classes like we do for unique_ptr / scoped_ptr.

shared_ptr is the same base base::linked_ptr as far as I understand it.

I believe almost all uses of base::linked_ptr can be deleted and replaced with movable types, since the normal use for this was to be able to put objects in standard containers without copying or using raw pointers. So I would advocate doing this replacement rather than using base::shared_ptr in this case.

My gut response is that I don't like shared_ptr and I'd prefer not to allow it now. One big thing we do with our RefCounted class is allow it to express which thread it should be deleted on. This is pretty important to us, so I don't think we can remove our own RefCounted class. And I think the different models about ownership will get confusing if we have both scoped_ptr and shared_ptr.

Brett

[Peter Kasting]

On Thu, Nov 12, 2015 at 3:16 PM, Daniel Cheng <dch...@chromium.org> wrote:

Forking this since this discussion is largely about shared_ptr.

Brett responded about shared_ptr directly in the original thread, but I'm moving my response to him here to try and keep shared_ptr stuff in this thread.

On Thu, Nov 12, 2015 at 3:52 PM, Brett Wilson <bre...@chromium.org> wrote:

My gut response is that I don't like shared_ptr and I'd prefer not to allow it now. One big thing we do with our RefCounted class is allow it to express which thread it should be deleted on. This is pretty important to us, so I don't think we can remove our own RefCounted class.

I worry a bit about code that does this, though.  In most of the cases I've seen, use of this functionality actually means that the code isn't sufficiently robust against some set of circumstances, and Bad Things can happen.  (I'm being vague because it's been a little while so I can't remember the details of specific examples.)

I'm of two minds here.  On one hand, I would really prefer to use the STL if at all possible and avoid our own classes that basically tackle the same problem.  For this reason, I really would prefer shared_ptr over scoped_refptr etc. as long as there's not a significant performance loss.

On the other hand, I kind of like that today you can't refcount a class unless the class allows it by inheriting from RefCounted.  It helps prevent people from refcounting in general, which is an explicit goal.  shared_ptr's ability to make anything refcounted seems like an invitation to bugginess.  This makes me want to disallow it.

PK

[Jeffrey Yasskin]

On Thu, Nov 12, 2015 at 3:52 PM, Brett Wilson <bre...@chromium.org> wrote:

> On Thu, Nov 12, 2015 at 2:24 PM, Nico Weber <tha...@chromium.org> wrote:
>>
>> I too think it makes sense to handle unique_ptr and shared_ptr separately.
>>

>> Allowing unique_ptr early makes sense to me (for quic if nothing else),
>> and https://codereview.chromium.org/1432193002/ seems like a good way to get
>> there.
>>
>> WrapUnique() also sounds good to me. Is it feasible to mass-replace
>> "make_scoped_ptr" to "wrap_scoped_ptr" to keep names similar? (/me shakes
>> fist at standard for giving make_unique gratuitously different behavior than
>> make_pair etc).
>>

>> I suppose shared_ptr is morally equivalent to scoped_refptr, so we'd need
>> to have some story around these two classes like we do for unique_ptr /
>> scoped_ptr.
>
>

> shared_ptr is the same base base::linked_ptr as far as I understand it.

~linked_ptr() takes O(refcount) time, while ~shared_ptr is O(1).
linked_ptr isn't thread-safe, while shared_ptr is. linked_ptr doesn't
require allocating a control block, while shared_ptr does (although
make_shared can allocate the control block adjacent to the object).
linked_ptr doesn't allow weak references, while shared_ptr does.
Otherwise, sure, they're both shared ownership mechanisms.

> My gut response is that I don't like shared_ptr and I'd prefer not to allow
> it now. One big thing we do with our RefCounted class is allow it to express
> which thread it should be deleted on. This is pretty important to us, so I

> don't think we can remove our own RefCounted class. And I think the
> different models about ownership will get confusing if we have both
> scoped_ptr and shared_ptr.

shared_ptr() has a deleter argument that can be used to pick the
destruction thread:
http://en.cppreference.com/w/cpp/memory/shared_ptr/shared_ptr

The main difference here is again that RefCountedThreadSafe requires a
single behavior for a whole type, while shared_ptr works per-instance.

I suspect we should stick with RefCounted*, but please lets dismiss
shared_ptr for real reasons and not because we don't understand it.

Jeffrey

[Brett Wilson]

Yeah, philosophically, I think it's important to be able to know by looking at the class what type of memory management it expects.

As an example, one of the big things we use RefCounted for is for posting work to a background thread (among other options for using Bind). But if this became shared_ptr to keep the object alive across the post, it becomes too easy to screw up the memory management and using both manual deleting + shared_ptr (if my understanding of shared_ptr is correct. Our RefCounted class has a lot of protections that tell you right away when you screw up.

Brett

[Dana Jansens]

On Thu, Nov 12, 2015 at 3:14 PM, Matthew Dempsky <mdem...@chromium.org> wrote:

On Thu, Nov 12, 2015 at 3:13 PM, Nico Weber <tha...@chromium.org> wrote:

I think we should have a single guideline for stuff in src.git, else things will get super confusing.

So we need to split sandbox/linux into bauxite.git to be able to use std::shared_ptr?

I would prefer that also, until/unless we decide to use it everywhere. Per-module style guide rules seems like a very slippery slope to spaghetti code.

[Nico Weber]

On Thu, Nov 12, 2015 at 1:15 PM, Daniel Cheng <dch...@chromium.org> wrote:

I think we should consider std::shared_ptr separately.

I had a draft for std::unique_ptr, but since the thread already started, I'll just dump it here instead. std::unique_ptr is blocked on std::move(), since it's not as useful without it. A few things that I think are worth highlighting:

• converting between std::unique_ptr and scoped_ptr: in order to allow people to start using it incrementally, it's probably useful to have a way to convert back and forth between them. https://codereview.chromium.org/1432193002 is a proposal that would allow this.
• std::unique_ptr and base::Bind: base::Bind() and base::Passed() depends on the custom macros for move semantics, which add a Pass() member to movable types. One option is to use template metaprogramming magic to make base::Passed() magically use std::move() for types with move constructors/operators. Another is to add more magic to base::Bind() so it can implicitly convert std::unique_ptr to scoped_ptr.
  

(IIRC this doesn't work well because move constructors don't guarantee to zero out the moved-from object. It's true for scoped_ptr and I think unique_ptr and hopefully our move-emulated objects we currently have, but not for all movable types in general.)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKr3LT1xQT9t6aMbKepPRb6UbGUy6rpfomLG6QEhMHQJhA%40mail.gmail.com.

[James Robinson]

On Thu, Nov 12, 2015 at 5:49 PM, Nico Weber <tha...@chromium.org> wrote:

On Thu, Nov 12, 2015 at 1:15 PM, Daniel Cheng <dch...@chromium.org> wrote:

I think we should consider std::shared_ptr separately.

I had a draft for std::unique_ptr, but since the thread already started, I'll just dump it here instead. std::unique_ptr is blocked on std::move(), since it's not as useful without it. A few things that I think are worth highlighting:

• converting between std::unique_ptr and scoped_ptr: in order to allow people to start using it incrementally, it's probably useful to have a way to convert back and forth between them. https://codereview.chromium.org/1432193002 is a proposal that would allow this.
• std::unique_ptr and base::Bind: base::Bind() and base::Passed() depends on the custom macros for move semantics, which add a Pass() member to movable types. One option is to use template metaprogramming magic to make base::Passed() magically use std::move() for types with move constructors/operators. Another is to add more magic to base::Bind() so it can implicitly convert std::unique_ptr to scoped_ptr.
  

(IIRC this doesn't work well because move constructors don't guarantee to zero out the moved-from object. It's true for scoped_ptr and I think unique_ptr and hopefully our move-emulated objects we currently have, but not for all movable types in general.)

The moved-from state for std::unique_ptr and for base' scoped_ptr is defined to be nullptr (c++11 20.7.1.4 for std::unique_ptr).  In general the moved-from state is not guaranteed to be anything in particular except that it will always be valid (i.e. safe to assign to or destroy).

- James

[Vladimir Levin]

On Thu, Nov 12, 2015 at 4:13 PM, 'Brett Wilson' via cxx <c...@chromium.org> wrote:

On Thu, Nov 12, 2015 at 4:07 PM, Peter Kasting <pkas...@google.com> wrote:

On Thu, Nov 12, 2015 at 3:16 PM, Daniel Cheng <dch...@chromium.org> wrote:

Forking this since this discussion is largely about shared_ptr.

Brett responded about shared_ptr directly in the original thread, but I'm moving my response to him here to try and keep shared_ptr stuff in this thread.

On Thu, Nov 12, 2015 at 3:52 PM, Brett Wilson <bre...@chromium.org> wrote:

My gut response is that I don't like shared_ptr and I'd prefer not to allow it now. One big thing we do with our RefCounted class is allow it to express which thread it should be deleted on. This is pretty important to us, so I don't think we can remove our own RefCounted class.

I worry a bit about code that does this, though.  In most of the cases I've seen, use of this functionality actually means that the code isn't sufficiently robust against some set of circumstances, and Bad Things can happen.  (I'm being vague because it's been a little while so I can't remember the details of specific examples.)

I'm of two minds here.  On one hand, I would really prefer to use the STL if at all possible and avoid our own classes that basically tackle the same problem.  For this reason, I really would prefer shared_ptr over scoped_refptr etc. as long as there's not a significant performance loss.

On the other hand, I kind of like that today you can't refcount a class unless the class allows it by inheriting from RefCounted.  It helps prevent people from refcounting in general, which is an explicit goal.  shared_ptr's ability to make anything refcounted seems like an invitation to bugginess.  This makes me want to disallow it.

Yeah, philosophically, I think it's important to be able to know by looking at the class what type of memory management it expects.

FWIW, there are other ways to achieve this. For example, 

class Foo {

 public:

   static std::shared_ptr<Foo> Create();

   ...

 private:

   Foo();

   ...

};

This would say to me that this class expects to be shared.  That is to say, we can still make sure that classes that must be ref counted can indicate this. However, the difference with a shared_ptr is that it's much harder to enforce that every class that is going to be used in a shared_ptr must mark itself as such. Is the latter part as important?

To put it differently, if a class has a static create that instead returns a std::unique_ptr<Foo>, then it could still be used in a shared_ptr. Is this going to cause bugs? If so then maybe we can hold off on shared_ptr (and do you have examples of where that might be a problem? :) )

As a separate point, we also have some code that starts out with a scoped_refptr, then passes a raw pointer down somewhere and whatever class receives the raw pointer puts it in another scoped_refptr. That's fine when ref counting is done in the class, but with shared_ptrs, if I'm not mistaken, this is going to cause a double free, since two blocks of ref counting would be allocated. I think if we lean towards allowing shared_ptr, then we at least need an audit of current usages first.

As an example, one of the big things we use RefCounted for is for posting work to a background thread (among other options for using Bind). But if this became shared_ptr to keep the object alive across the post, it becomes too easy to screw up the memory management and using both manual deleting + shared_ptr (if my understanding of shared_ptr is correct. Our RefCounted class has a lot of protections that tell you right away when you screw up.

Brett

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CABiGVV9Vnsi5sgMo5kfEcgT9Tku4r5cxD6uCN4QCPZ_HhpY%2BeA%40mail.gmail.com.

[Daniel Cheng]

Is allowing this to work with real movable types any worse than what we have today? You can already run a Callback with a move-only emulation type more than once. Doing so with the current system is probably a programmer error, and would remain so with real movable types.

Daniel

 

- James

[Dana Jansens]

I'd like to be precise in our terminology here. Callback will not do anything but copy _movable_ types. It will only call .Pass() (aka move()) on _move-only_ types that were made with our move.h macro (cuz it makes them move-only).

I agree that you can do so more than once today, which is a programmer error. There are relatively few move-only types in Chromium compared to types in the standard library, and probably even fewer are passed through Callbacks. Hopefully they would cause an obvious crash by nulling things instead of swapping things in the Move constructor, but of course there's no guarantees.

Using Callback with std::unique_ptr would be "okay" and fail in the same ways that scoped_ptr would today. But it won't compile without changing Callback because it would try to copy it.

I think writing Passed support for unique_ptr that converts to a scoped_ptr seems okay because that's just basically a spelling change for people using it. I do not support adding general move-only or move-able support to base::Callback as it is today (copyable + repeat-callable) and think that we need to revisit those things in implementing support for general move-only types (outside of our special macro).

Which leads to temporary guidance probably being something like, if you want to pass a move-only type through a callback, you need to wrap it in a std::unique_ptr. Perhaps until Bind/Callback is fixed, it would be nice to actually migrate other move-only types off of the base/move.h macro, which would cause them to require wrapping in a scoped_ptr/unique_ptr to go through Callback. Then we have some really well-defined behaviour there (definitely a nullptr crash on misuse) until we make things better.

So.. all of this is to say, I do not think we should block unique_ptr on changing Callback, people can convert before calling Passed if they need to. And I do think we should make implicit conversions to scoped_ptr inside the Callback system for now so people can do this transition more seamlessly as well.

Because scoped_ptr and unique_ptr are so alike, I think that discussions about general move-only or movable types and Callback probably shouldn't guide the decision about how or where to use std::unique_ptr in our codebase.

 

Daniel

 

- James

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKrbhiX9knK%2BJzK3A46Jbup0RwZa2rW10Bc7XY4Ca2fc2g%40mail.gmail.com.

[Chris Blume]

On the other hand, I kind of like that today you can't refcount a class unless the class allows it by inheriting from RefCounted.  It helps prevent people from refcounting in general, which is an explicit goal.  shared_ptr's ability to make anything refcounted seems like an invitation to bugginess.  This makes me want to disallow it.

I agree with discouraging ref counting in general. And I understand that making ref counting more difficult can be a very very good way to discourage it. However, I strongly dislike the idea of any type not related to memory management having to deal with memory management. Especially when it involves inheritance. Imagine if you could not put an int on the stack because it was ref counted in some case. So instead either all ints are ref counted or we have two types (one for ref counted and one not). Neither of these options is good and it stems from the type assuming too much about its usage. Let the type care only about the one thing it should care about. Compositing restrictions for each usage is much easier (I need an int and I need it ref counted).

Given these two evils, I feel like shared_ptr is the lesser.

 

As a separate point, we also have some code that starts out with a scoped_refptr, then passes a raw pointer down somewhere and whatever class receives the raw pointer puts it in another scoped_refptr. That's fine when ref counting is done in the class, but with shared_ptrs, if I'm not mistaken, this is going to cause a double free, since two blocks of ref counting would be allocated. I think if we lean towards allowing shared_ptr, then we at least need an audit of current usages first.

This sounds to me like a problem that should be fixed.

It sounds like it happens to work now because the type has the ref count baked in. But the design requires it be ref counted and doesn't explicitly state that. It relies on the type to enforce that.

Since the design requires the type be ref counted, it could explicitly state that by taking a shared_ptr instead of a raw pointer.

[Vladimir Levin]

On Wed, Nov 18, 2015 at 6:23 AM, Chris Blume <cbl...@google.com> wrote:

On the other hand, I kind of like that today you can't refcount a class unless the class allows it by inheriting from RefCounted.  It helps prevent people from refcounting in general, which is an explicit goal.  shared_ptr's ability to make anything refcounted seems like an invitation to bugginess.  This makes me want to disallow it.

I agree with discouraging ref counting in general. And I understand that making ref counting more difficult can be a very very good way to discourage it. However, I strongly dislike the idea of any type not related to memory management having to deal with memory management. Especially when it involves inheritance. Imagine if you could not put an int on the stack because it was ref counted in some case. So instead either all ints are ref counted or we have two types (one for ref counted and one not). Neither of these options is good and it stems from the type assuming too much about its usage. Let the type care only about the one thing it should care about. Compositing restrictions for each usage is much easier (I need an int and I need it ref counted).

Given these two evils, I feel like shared_ptr is the lesser.

+1, I very much dislike having to decide whether a class should be ref counted or not when writing the class itself, instead of when deciding (or changing) its usage later. 

 

 

As a separate point, we also have some code that starts out with a scoped_refptr, then passes a raw pointer down somewhere and whatever class receives the raw pointer puts it in another scoped_refptr. That's fine when ref counting is done in the class, but with shared_ptrs, if I'm not mistaken, this is going to cause a double free, since two blocks of ref counting would be allocated. I think if we lean towards allowing shared_ptr, then we at least need an audit of current usages first.

This sounds to me like a problem that should be fixed.

It sounds like it happens to work now because the type has the ref count baked in. But the design requires it be ref counted and doesn't explicitly state that. It relies on the type to enforce that.

Since the design requires the type be ref counted, it could explicitly state that by taking a shared_ptr instead of a raw pointer.

I agree, I was only pointing it out that this exists and we have to be careful not to just s/scoped_refptr/shared_ptr/.  

[Ryan Sleevi]

On Wed, Nov 18, 2015 at 10:59 AM, 'Vladimir Levin' via cxx <c...@chromium.org> wrote:

+1, I very much dislike having to decide whether a class should be ref counted or not when writing the class itself, instead of when deciding (or changing) its usage later. 

Given the sheer number of reference counting issues I've seen - either through incoming reviews or bugs that make it out - I would argue that "deciding whether a class should be ref-counted or not" is very much a decision you should be making up front, at the design time, because it materially affects everything around it.

Switching to reference counting later is almost _always_ an anti-pattern, and using reference counting early in your design is... well, almost always an anti-pattern too :)

At the risk of "+1/-1" wars, this is as much an intentional design decision as could be, no different than deciding whether or not you want to allow inheritance, whether to make your class concrete or an interface, whether or not it's a 'public' or 'private' API, and all of the other attendant software design issues.

[Brett Wilson]

Yea, I fear having every individual programmer decide at time of use whether a given class should be refcounted or not will be disastrous. I think reference counting generally works OK in Chrome today.

Brett

[Vladimir Levin]

On Wed, Nov 18, 2015 at 11:06 AM, Brett Wilson <bre...@google.com> wrote:

On Wed, Nov 18, 2015 at 11:03 AM, Ryan Sleevi <rsl...@chromium.org> wrote:

On Wed, Nov 18, 2015 at 10:59 AM, 'Vladimir Levin' via cxx <c...@chromium.org> wrote:

+1, I very much dislike having to decide whether a class should be ref counted or not when writing the class itself, instead of when deciding (or changing) its usage later. 

Given the sheer number of reference counting issues I've seen - either through incoming reviews or bugs that make it out - I would argue that "deciding whether a class should be ref-counted or not" is very much a decision you should be making up front, at the design time, because it materially affects everything around it.

I assume these bugs were using our current ref counting methods? If that's the case, then maybe it's a case for changing the current system. :) On a more serious note, I agree that ref counting is hard and it should be used rarely and carefully. I just feel that baking that knowledge into the class at the get-go is not good. If someone decides that a class has to be ref counted, that forces all callers to use refcounting. Even if those callers would otherwise prefer to create a scoped_ptr instead.

 

Switching to reference counting later is almost _always_ an anti-pattern, and using reference counting early in your design is... well, almost always an anti-pattern too :)

At the risk of "+1/-1" wars, this is as much an intentional design decision as could be, no different than deciding whether or not you want to allow inheritance, whether to make your class concrete or an interface, whether or not it's a 'public' or 'private' API, and all of the other attendant software design issues.

Yea, I fear having every individual programmer decide at time of use whether a given class should be refcounted or not will be disastrous. I think reference counting generally works OK in Chrome today.

I'm not sure I understand why it would be disastrous. Can you elaborate? If the caller is the one that's creating the object then presumably the caller knows why it's creating the object and it knows whether that particular object should be ref counted. 

As mentioned earlier, the class itself can guide (or rather force) the caller by exposing a create function that returns a shared_ptr (which I think is pretty important when the class is using shared_from_this). 

The problem, as cblume mentioned, is that if there are situations where you want one object not to ref count and the other one to ref count. Right now that's only possible with necessarily ref counting and having a ref count of 1. It works, but it doesn't specify intent very well. (As a disclaimer, I don't actually have specific examples of these situations)

 

Brett

[Peter Kasting]

On Wed, Nov 18, 2015 at 6:23 AM, Chris Blume <cbl...@google.com> wrote:

On the other hand, I kind of like that today you can't refcount a class unless the class allows it by inheriting from RefCounted.  It helps prevent people from refcounting in general, which is an explicit goal.  shared_ptr's ability to make anything refcounted seems like an invitation to bugginess.  This makes me want to disallow it.

I agree with discouraging ref counting in general. And I understand that making ref counting more difficult can be a very very good way to discourage it. However, I strongly dislike the idea of any type not related to memory management having to deal with memory management. Especially when it involves inheritance. Imagine if you could not put an int on the stack because it was ref counted in some case. So instead either all ints are ref counted or we have two types (one for ref counted and one not). Neither of these options is good and it stems from the type assuming too much about its usage. Let the type care only about the one thing it should care about. Compositing restrictions for each usage is much easier (I need an int and I need it ref counted).

I think the choice of example type here is misleading.  An int is both conceptually simple and hugely broad in its applicability.  All kinds of code wants to use ints for all kinds of reasons, but what they actually do with them -- store a single value and retrieve it -- is very simple.

Refcounted classes in Chromium are almost always at the other end of those scales.  They hold complex functionality and they are usually used in a limited set of circumstances.  Being refcounted often affects class design choices like how deletion happens or what needs locking.  For all these reasons, it generally makes sense for the question of refcounting to not be decoupled from the class design.

Now, in a few cases I might agree with you.  Let's say we wanted a class to represent a complex number (like std::complex).  That's probably a class where refcounting decisions can safely be made outside the class.  But I don't believe most Chromium code is like that.

PK

[Ruud van Asseldonk]

Refcounted classes in Chromium are almost always at the other end of those scales.  They hold complex functionality and they are usually used in a limited set of circumstances.  Being refcounted often affects class design choices like how deletion happens or what needs locking.  For all these reasons, it generally makes sense for the question of refcounting to not be decoupled from the class design.

To give a concrete example: here an instance could have been stack allocated perfectly well, but because the class is refcounted it now incurs a heap allocation.

[Peter Kasting]

On Wed, Nov 18, 2015 at 11:34 AM, Ruud van Asseldonk <ru...@google.com> wrote:

Refcounted classes in Chromium are almost always at the other end of those scales.  They hold complex functionality and they are usually used in a limited set of circumstances.  Being refcounted often affects class design choices like how deletion happens or what needs locking.  For all these reasons, it generally makes sense for the question of refcounting to not be decoupled from the class design.

To give a concrete example: here an instance could have been stack allocated perfectly well, but because the class is refcounted it now incurs a heap allocation.

I don't know anything about TraceValue and why ConvertibleToTraceFormat is RefCounted, so I can't speak concretely about this case.

I'm certain there are cases in Chromium today where we refcount needlessly and it would have been safe to not refcount.  That's why I was careful to use words like "generally" when making my argument.

PK 

[Ryan Sleevi]

On Wed, Nov 18, 2015 at 11:20 AM, Vladimir Levin <vmp...@google.com> wrote:

I assume these bugs were using our current ref counting methods? If that's the case, then maybe it's a case for changing the current system. :)

No. Lifetimes are hard - reference counting expressly serves to attempt to decouple sane lifetime management.

https://groups.google.com/a/chromium.org/d/msg/chromium-dev/Oy3uuiTSwDk/5UDyRJcpJsUJ , while being about WeakPtr, certainly discusses a lot of the challenges in lifetime management and coupling.

Also, https://groups.google.com/a/chromium.org/d/msg/chromium-dev/BLBuqeq29T4/Lm2xym_IYxcJ

While I'm totally in favour of loosening coupling when possible, we have to be careful that when things are strongly coupled, we actually loosen coupling, rather than just obscure coupling. Reference counting is one way in which coupling is obscured, not lessened.

 

On a more serious note, I agree that ref counting is hard and it should be used rarely and carefully. I just feel that baking that knowledge into the class at the get-go is not good. If someone decides that a class has to be ref counted, that forces all callers to use refcounting.

Correct. Which

1) Forces careful consideration as to the lifetimes of both objects (conceptually) and instances (as concrete things with lifetime management)

2) Is a net-negative enough to discourage reference counting.

 

Even if those callers would otherwise prefer to create a scoped_ptr instead.

 

I'm not sure I understand why it would be disastrous. Can you elaborate? If the caller is the one that's creating the object then presumably the caller knows why it's creating the object and it knows whether that particular object should be ref counted. 

In practice, this tends to work out extremely poorly.

A classic example it's let's imagine

class Foo {

 public:

  Foo(Bar* bar, Baz* baz);

};

This tells you some things about lifetime management, but not everything.

The immediate read should be that "Bar and Baz should remain valid for at least as long as the constructor, but likely as long as the object"

If that was perhaps implicitly guaranteed in some way - for example, if Baz was having ownership transferred, your reviewer should push on you to make that clearer:

Foo(Bar* bar, scoped_ptr<Baz> baz);

 

But we at least know that absent other information, Bar MUST remain valid for the duration of Foo. There's a tight coupling of those lifetimes.

Now, regardless of whether you're using RefCounted or shared_ptr<>, the only way to safely use 'foo' is to ensure that everything that may take a reference to Foo outlives Bar. That's hard. Hard for people and hard for machines to guarantee - and that's why reference counting is extremely dangerous.

The current pattern - deriving from RefCounted - makes this somewhat easier. This is because you MUST make a decision, at design time, whether or not Foo should be refcounted. If it is, that immediately forces Foo to be un-stack-allocatable (we have a clang plugin to make sure no public dtors exist throughout the inheritance chain of Foo - or anything that implements Foo - to ensure this). This means that everyone _must_ write something like

scoped_refptr<Foo> foo(new Foo(bar));

And that's a big sign, at time of writing the code, that Foo/Bar are quite coupled now. It should hopefully raise red flags - and minimally raise the standard - to make sure at all places that "foo" (the instance) outlives "bar".

Now, let's say we embraced shared_ptr, which is effectively "recipient decides" rather than "creator decides" reference counting. Let's say Foo is

class Foo {

 public:

  explicit Foo(Bar* bar);

 private:

  shared_ptr<Bar> bar;

};

From reading the constructor, one would again presume the lifetime guarantees - that Bar (and anything Bar depends on) must outlive Foo. Except now the contract has been changed - Bar (and dependents) doesn't just have Foo, they have to outlive everything that might have taken the shared_ptr.

If Foo ever gives out the shared_ptr<> to 'something' (which is inevitably why you make it shared - so you can share), then Foo no longer can make any guarantees as to the lifetime of Bar, nor can anyone writing Bar (such as adding a new dependency for Bar) know what the lifetimes or risks are.

Lifetime issues are subtle and hard to trace. shared_ptr<>, while appealing in many ways, can further obfuscate the lifetimes and guarantees - which makes it easier to write buggy software. RefCounted is not perfect in this regard - the fact that anyone can grab the reference to the object (RefCounted has an implied enable_shared_from_this) makes it complicated, and Blink's types (RefPtr vs OwnRefPtr, etc) try to solve _that_ problem as well.

Perfect? No. But certainly why reference counting is hard :)

As mentioned earlier, the class itself can guide (or rather force) the caller by exposing a create function that returns a shared_ptr (which I think is pretty important when the class is using shared_from_this). 

The problem, as cblume mentioned, is that if there are situations where you want one object not to ref count and the other one to ref count. Right now that's only possible with necessarily ref counting and having a ref count of 1.

Well, no, there's RefCountedData, which conceptually matches shared_ptr in that's an external ref count rather than an intrusive reference count.

 

It works, but it doesn't specify intent very well. (As a disclaimer, I don't actually have specific examples of these situations)

I have yet to see such a situation in practice come up which didn't already have other design issues (either in lifetimes or in coupling). That's anecdotal, for sure, but sometimes the best way to make progress is with concrete use cases. We have plenty of concrete evidence of bugs and developer confusion with reference counting, so I'm not at all keen to make it easier for the sake of potential use cases we can't articulate :/

[David Benjamin]

On Wed, Nov 18, 2015 at 2:47 PM Ryan Sleevi <rsl...@chromium.org> wrote:

I'm not sure I understand why it would be disastrous. Can you elaborate? If the caller is the one that's creating the object then presumably the caller knows why it's creating the object and it knows whether that particular object should be ref counted. 

In practice, this tends to work out extremely poorly.

A classic example it's let's imagine

class Foo {

 public:

  Foo(Bar* bar, Baz* baz);

};

This tells you some things about lifetime management, but not everything.

The immediate read should be that "Bar and Baz should remain valid for at least as long as the constructor, but likely as long as the object"

If that was perhaps implicitly guaranteed in some way - for example, if Baz was having ownership transferred, your reviewer should push on you to make that clearer:

Foo(Bar* bar, scoped_ptr<Baz> baz);

 

But we at least know that absent other information, Bar MUST remain valid for the duration of Foo. There's a tight coupling of those lifetimes.

Now, regardless of whether you're using RefCounted or shared_ptr<>, the only way to safely use 'foo' is to ensure that everything that may take a reference to Foo outlives Bar. That's hard. Hard for people and hard for machines to guarantee - and that's why reference counting is extremely dangerous.

The current pattern - deriving from RefCounted - makes this somewhat easier. This is because you MUST make a decision, at design time, whether or not Foo should be refcounted. If it is, that immediately forces Foo to be un-stack-allocatable (we have a clang plugin to make sure no public dtors exist throughout the inheritance chain of Foo - or anything that implements Foo - to ensure this). This means that everyone _must_ write something like

scoped_refptr<Foo> foo(new Foo(bar));

And that's a big sign, at time of writing the code, that Foo/Bar are quite coupled now. It should hopefully raise red flags - and minimally raise the standard - to make sure at all places that "foo" (the instance) outlives "bar".

Now, let's say we embraced shared_ptr, which is effectively "recipient decides" rather than "creator decides" reference counting. Let's say Foo is

class Foo {

 public:

  explicit Foo(Bar* bar);

 private:

  shared_ptr<Bar> bar;

};

From reading the constructor, one would again presume the lifetime guarantees - that Bar (and anything Bar depends on) must outlive Foo. Except now the contract has been changed - Bar (and dependents) doesn't just have Foo, they have to outlive everything that might have taken the shared_ptr.

If Foo ever gives out the shared_ptr<> to 'something' (which is inevitably why you make it shared - so you can share), then Foo no longer can make any guarantees as to the lifetime of Bar, nor can anyone writing Bar (such as adding a new dependency for Bar) know what the lifetimes or risks are.

[Disclaimer: all my C++ is done in Chromium, so I've never actually worked with a codebase that uses shared_ptr. It's possible I'm completely wrong here.]

Is this actually true with shared_ptr? Unlike scoped_refptr, shared_ptr doesn't have an automatic (public!) enable_shared_from_this everywhere.

In scoped_refptr land, you can freely go from T* to scoped_refptr<T>. So you indeed have no clue what Foo is going to do to bar. But in shared_ptr land, you'd be using the shared_ptr<Bar>(Bar*) constructor, which is analogous to the unique_ptr<Bar>(Bar*) constructor which explicitly takes ownership of a raw pointer.

That is, this is just the same problem we had before we had Pass(). If we really wanted, we could have written:

class Foo {

 public:

  explicit Foo(unique_ptr<Bar> bar) : bar_(bar.release()) {}

 private:

  shared_ptr<Bar> bar_;

};

Though probably more likely it would have been (with appropriate std::move and/or const-reference line noise):

class Foo {

 public:

  explicit Foo(shared_ptr<Bar> bar) : bar_(bar) {}

 private:

  shared_ptr<Bar> bar_;

};

In both cases, we avoid the problem where the constructor made ownership unclear. Even if bar_ were a unique_ptr, it could still have passed ownership off to someone else and outlive Foo. Notably none of these type signatures would have allowed this situation (again, assuming we never enable_shared_from_this), which is I think what you were alluding to:

  shared_ptr<Bar> bar;

  Bar* non_owning_bar = bar.get();

  // ...

  Foo foo(non_owning_bar);

  // foo has secretly taken an additional reference to non_owning_bar.

Otherwise, I completely agree that ref-counting is evil and makes it impossible to handle ownership contracts in all the ways you mention. But I think shared_ptr might actually be less evil than RefCounted here.

David

 

Lifetime issues are subtle and hard to trace. shared_ptr<>, while appealing in many ways, can further obfuscate the lifetimes and guarantees - which makes it easier to write buggy software. RefCounted is not perfect in this regard - the fact that anyone can grab the reference to the object (RefCounted has an implied enable_shared_from_this) makes it complicated, and Blink's types (RefPtr vs OwnRefPtr, etc) try to solve _that_ problem as well.

Perfect? No. But certainly why reference counting is hard :)

As mentioned earlier, the class itself can guide (or rather force) the caller by exposing a create function that returns a shared_ptr (which I think is pretty important when the class is using shared_from_this). 

The problem, as cblume mentioned, is that if there are situations where you want one object not to ref count and the other one to ref count. Right now that's only possible with necessarily ref counting and having a ref count of 1.

Well, no, there's RefCountedData, which conceptually matches shared_ptr in that's an external ref count rather than an intrusive reference count.

 

It works, but it doesn't specify intent very well. (As a disclaimer, I don't actually have specific examples of these situations)

I have yet to see such a situation in practice come up which didn't already have other design issues (either in lifetimes or in coupling). That's anecdotal, for sure, but sometimes the best way to make progress is with concrete use cases. We have plenty of concrete evidence of bugs and developer confusion with reference counting, so I'm not at all keen to make it easier for the sake of potential use cases we can't articulate :/

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACvaWvY%2BmMRu1uYyN7vF5Y7t9eQXR2ppdcNiMZTJcmXgp_bRnA%40mail.gmail.com.

[Jeffrey Yasskin]

On Wed, Nov 18, 2015 at 6:23 AM, 'Chris Blume' via cxx <c...@chromium.org> wrote:
>> As a separate point, we also have some code that starts out with a
>> scoped_refptr, then passes a raw pointer down somewhere and whatever class
>> receives the raw pointer puts it in another scoped_refptr. That's fine when
>> ref counting is done in the class, but with shared_ptrs, if I'm not
>> mistaken, this is going to cause a double free, since two blocks of ref
>> counting would be allocated. I think if we lean towards allowing shared_ptr,
>> then we at least need an audit of current usages first.
>
>
> This sounds to me like a problem that should be fixed.
>
> It sounds like it happens to work now because the type has the ref count
> baked in. But the design requires it be ref counted and doesn't explicitly
> state that. It relies on the type to enforce that.
>
> Since the design requires the type be ref counted, it could explicitly state
> that by taking a shared_ptr instead of a raw pointer.

This tends to be difficult with callback-based code. If we have a
sequence of methods on an object, and the type is refcounted, you wind
up losing the shared-ness on each method call. To get it back, you'd
need to use static functions instead of instance methods:

[disclaimer: these classes have not been compiled, so probably have bugs]

class MyShared : public RefCountedThreadSafe<MyShared> {
public:
void Method1(int arg) {
// Bind doesn't require the make_scoped_refptr, but to be clear:
PostTask(base::Bind(&MyShared::Method2, make_scoped_refptr(this), arg));
}
void Method2(int arg) {
PostTask(base::Bind(&MyShared::Method3, make_scoped_refptr(this), arg));
}
void Method3(int arg) {
printf("%d", arg);
}
};

class MyShared {
public:
static std::shared_ptr<MyShared> create() { return
std::make_shared<MyShared>(); }
static void Method1(std::shared_ptr<MyShared> self, int arg) {
PostTask(base::Bind(&MyShared::Method2, std::move(self), arg));
}
static void Method2(std::shared_ptr<MyShared> self, int arg) {
PostTask(base::Bind(&MyShared::Method3, std::move(self), arg));
}
static void Method3(std::shared_ptr<MyShared> self, int arg) {
printf("%d", arg);
}
};

You can also inherit from std::enable_shared_from_this, but that adds
back the problem of folks reconstituting shared ownership from a raw
pointer.

class MyShared : public std::enable_shared_from_this {
public:
void Method1(int arg) {
// Bind doesn't require the make_scoped_refptr, but to be clear:
PostTask(base::Bind(&MyShared::Method2, shared_from_this(), arg));
}
void Method2(int arg) {
PostTask(base::Bind(&MyShared::Method3, shared_from_this(), arg));
}
void Method3(int arg) {
printf("%d", arg);
}
};

[Vladimir Levin]

On Wed, Nov 18, 2015 at 11:46 AM, Ryan Sleevi <rsl...@chromium.org> wrote:

On Wed, Nov 18, 2015 at 11:20 AM, Vladimir Levin <vmp...@google.com> wrote:

I assume these bugs were using our current ref counting methods? If that's the case, then maybe it's a case for changing the current system. :)

No. Lifetimes are hard - reference counting expressly serves to attempt to decouple sane lifetime management.

https://groups.google.com/a/chromium.org/d/msg/chromium-dev/Oy3uuiTSwDk/5UDyRJcpJsUJ , while being about WeakPtr, certainly discusses a lot of the challenges in lifetime management and coupling.

Also, https://groups.google.com/a/chromium.org/d/msg/chromium-dev/BLBuqeq29T4/Lm2xym_IYxcJ

While I'm totally in favour of loosening coupling when possible, we have to be careful that when things are strongly coupled, we actually loosen coupling, rather than just obscure coupling. Reference counting is one way in which coupling is obscured, not lessened.

 

On a more serious note, I agree that ref counting is hard and it should be used rarely and carefully. I just feel that baking that knowledge into the class at the get-go is not good. If someone decides that a class has to be ref counted, that forces all callers to use refcounting.

Correct. Which

1) Forces careful consideration as to the lifetimes of both objects (conceptually) and instances (as concrete things with lifetime management)

2) Is a net-negative enough to discourage reference counting.

 

Even if those callers would otherwise prefer to create a scoped_ptr instead.

 

I'm not sure I understand why it would be disastrous. Can you elaborate? If the caller is the one that's creating the object then presumably the caller knows why it's creating the object and it knows whether that particular object should be ref counted. 

In practice, this tends to work out extremely poorly.

A classic example it's let's imagine

class Foo {

 public:

  Foo(Bar* bar, Baz* baz);

};

This tells you some things about lifetime management, but not everything. 

The immediate read should be that "Bar and Baz should remain valid for at least as long as the constructor, but likely as long as the object"

Without any other knowledge, passing a non owning pointer to a ctor says to me "here's some stuff that you can use while constructing the object." In my mind this doesn't talk about lifetime guarantees at all (aside from being alive for the duration of the ctor -- we're assuming single threaded for these cases, right?). Bar maybe outlives foo, but baz maybe doesn't. The actual usage of this might dictate that bar has to outlive baz, but just from the code you have there it's unclear.

 

If that was perhaps implicitly guaranteed in some way - for example, if Baz was having ownership transferred, your reviewer should push on you to make that clearer:

Foo(Bar* bar, scoped_ptr<Baz> baz);

 

But we at least know that absent other information, Bar MUST remain valid for the duration of Foo. There's a tight coupling of those lifetimes.

In this case, bar is the same as before, baz is now an owning pointer, where the ownership is transferred.

 

Now, regardless of whether you're using RefCounted or shared_ptr<>, the only way to safely use 'foo' is to ensure that everything that may take a reference to Foo outlives Bar. That's hard. Hard for people and hard for machines to guarantee - and that's why reference counting is extremely dangerous.

I agree.

 

The current pattern - deriving from RefCounted - makes this somewhat easier. This is because you MUST make a decision, at design time, whether or not Foo should be refcounted. If it is, that immediately forces Foo to be un-stack-allocatable (we have a clang plugin to make sure no public dtors exist throughout the inheritance chain of Foo - or anything that implements Foo - to ensure this). This means that everyone _must_ write something like

scoped_refptr<Foo> foo(new Foo(bar));

And that's a big sign, at time of writing the code, that Foo/Bar are quite coupled now. It should hopefully raise red flags - and minimally raise the standard - to make sure at all places that "foo" (the instance) outlives "bar".

This is where I fundamentally disagree.  

"scoped_refptr<Foo> foo(new Foo(bar));" does not say anything new to me that "Foo foo(bar);" doesn't. It's a lot of boiler plate for a thing that will be destroyed at the end of scope. Using intrusive refcounting however _forces_ me to refcount. If the hope to reduce refcounting in the codebase, then that seems to be a step back. This doesn't give me any new hints or information about the lifetime of bar relative to foo.

 

Now, let's say we embraced shared_ptr, which is effectively "recipient decides" rather than "creator decides" reference counting. Let's say Foo is

class Foo {

 public:

  explicit Foo(Bar* bar);

 private:

  shared_ptr<Bar> bar;

};

From reading the constructor, one would again presume the lifetime guarantees - that Bar (and anything Bar depends on) must outlive Foo. Except now the contract has been changed - Bar (and dependents) doesn't just have Foo, they have to outlive everything that might have taken the shared_ptr.

This is the type of situation that I was describing that leads to double free bugs. When given a non-owning raw pointer, you must not put it in a shared_ptr, simply because that will create a new ref counting block. If this raw pointer was already shared somewhere else, then you will have two distinct ref counts to it and that's bad. I completely agree that intrusive ref counting solves this problem. This is complete aside from lifetime issues though.

 

If Foo ever gives out the shared_ptr<> to 'something' (which is inevitably why you make it shared - so you can share), then Foo no longer can make any guarantees as to the lifetime of Bar, nor can anyone writing Bar (such as adding a new dependency for Bar) know what the lifetimes or risks are.

I'm a bit confused here, because the same argument holds if Foo hands out scoped_refptr no?

[Jeffrey Yasskin]

On Wed, Nov 18, 2015 at 11:46 AM, Ryan Sleevi <rsl...@chromium.org> wrote:
> there's RefCountedData, which conceptually matches shared_ptr in
> that's an external ref count rather than an intrusive reference count.

O RLY? What do you think of replacing RefCountedData with
std::shared_ptr? ;) It might make sense to ban the std::shared_ptr(T*)
constructor in favor of std::make_shared<T> in order to make sure the
control block is allocated with the object...

We'd still have the problems of shared_ptr and its control block being
~twice as big as scoped_refptr and the refcount in RefCounted.
RefCountedData is thread-safe, so that isn't a disadvantage.

Jeffrey

[Nico Weber]

unique_ptr works with Callback as of three days ago. Were there other blockers, or can we allow unique_ptr now? Or are there other things we were waiting for?

[Daniel Cheng]

I'm hoping to land https://codereview.chromium.org/1532433002 as well (for our pre C++14 version of std::make_unique).

It would also be nice to be able to seamlessly convert to/from scoped_ptr, but without VS2015, it's going to be fairly verbose so (IMO) adding a conversion helper doesn't add much. IMO, we should just encourage new code to use std::unique_ptr and work on finishing the migration away from scoped_ptr in parallel.

Daniel

[Dana Jansens]

On Thu, Dec 17, 2015 at 10:38 AM, Daniel Cheng <dch...@chromium.org> wrote:

I'm hoping to land https://codereview.chromium.org/1532433002 as well (for our pre C++14 version of std::make_unique).

It would also be nice to be able to seamlessly convert to/from scoped_ptr, but without VS2015, it's going to be fairly verbose so (IMO) adding a conversion helper doesn't add much. IMO, we should just encourage new code to use std::unique_ptr and work on finishing the migration away from scoped_ptr in parallel.

Being able to convert without losing the deleter (release() will drop it) would be nice.

For the record, some attempts at converting between were done here https://codereview.chromium.org/1432193002/.

Implicitly converting scoped_ptr to unique_ptr should be done with an rvalue method, but ref-qualifiers do not work in MSVC 2013: https://msdn.microsoft.com/en-us/library/hh567368.aspx#corelanguagetable.

At that time we were considering trying to make scoped_ptr a subset of unique_ptr (matching exactly except not allowing reference deleters), and then making scoped_ptr a typedef for unique_ptr. I forget where that derailed..

I think we could still maybe consider doing implicit conversion from unique_ptr to scoped_ptr, and provide a helper method to convert a scoped_ptr to unique_ptr (not a member method so that it can take a scoped_ptr&&).

I don't think there's any reason to ban using std::unique_ptr right now, but converting existing code is still going to be a bit awkward I think without some conversion help.

[Daniel Cheng]

On Thu, Dec 17, 2015 at 12:11 PM Dana Jansens <dan...@google.com> wrote:

On Thu, Dec 17, 2015 at 10:38 AM, Daniel Cheng <dch...@chromium.org> wrote:

I'm hoping to land https://codereview.chromium.org/1532433002 as well (for our pre C++14 version of std::make_unique).

It would also be nice to be able to seamlessly convert to/from scoped_ptr, but without VS2015, it's going to be fairly verbose so (IMO) adding a conversion helper doesn't add much. IMO, we should just encourage new code to use std::unique_ptr and work on finishing the migration away from scoped_ptr in parallel.

Being able to convert without losing the deleter (release() will drop it) would be nice.

For the record, some attempts at converting between were done here https://codereview.chromium.org/1432193002/.

Implicitly converting scoped_ptr to unique_ptr should be done with an rvalue method, but ref-qualifiers do not work in MSVC 2013: https://msdn.microsoft.com/en-us/library/hh567368.aspx#corelanguagetable.

At that time we were considering trying to make scoped_ptr a subset of unique_ptr (matching exactly except not allowing reference deleters), and then making scoped_ptr a typedef for unique_ptr. I forget where that derailed..

That's still in process: bulk conversion of Pass() to std::move() is the largest remaining task that I know of... unless we missed some other subtle difference between std::unique_ptr and scoped_ptr.

 

I think we could still maybe consider doing implicit conversion from unique_ptr to scoped_ptr, and provide a helper method to convert a scoped_ptr to unique_ptr (not a member method so that it can take a scoped_ptr&&).

I don't think there's any reason to ban using std::unique_ptr right now, but converting existing code is still going to be a bit awkward I think without some conversion help.

The reason I'm not a fan of the conversion helpers is because it's something we'll have to come back and clean up. I think the Pass() work will be done in 1 week, at which point, we can see just how many things explode with a typedef. If fixing the explosions doesn't seem feasible, then it would be reasonable to revisit this.

Daniel

[Dana Jansens]

On Thu, Dec 17, 2015 at 5:12 PM, Daniel Cheng <dch...@chromium.org> wrote:

On Thu, Dec 17, 2015 at 12:11 PM Dana Jansens <dan...@google.com> wrote:

On Thu, Dec 17, 2015 at 10:38 AM, Daniel Cheng <dch...@chromium.org> wrote:

I'm hoping to land https://codereview.chromium.org/1532433002 as well (for our pre C++14 version of std::make_unique).

It would also be nice to be able to seamlessly convert to/from scoped_ptr, but without VS2015, it's going to be fairly verbose so (IMO) adding a conversion helper doesn't add much. IMO, we should just encourage new code to use std::unique_ptr and work on finishing the migration away from scoped_ptr in parallel.

Being able to convert without losing the deleter (release() will drop it) would be nice.

For the record, some attempts at converting between were done here https://codereview.chromium.org/1432193002/.

Implicitly converting scoped_ptr to unique_ptr should be done with an rvalue method, but ref-qualifiers do not work in MSVC 2013: https://msdn.microsoft.com/en-us/library/hh567368.aspx#corelanguagetable.

At that time we were considering trying to make scoped_ptr a subset of unique_ptr (matching exactly except not allowing reference deleters), and then making scoped_ptr a typedef for unique_ptr. I forget where that derailed..

That's still in process: bulk conversion of Pass() to std::move() is the largest remaining task that I know of... unless we missed some other subtle difference between std::unique_ptr and scoped_ptr.

 

I think we could still maybe consider doing implicit conversion from unique_ptr to scoped_ptr, and provide a helper method to convert a scoped_ptr to unique_ptr (not a member method so that it can take a scoped_ptr&&).

I don't think there's any reason to ban using std::unique_ptr right now, but converting existing code is still going to be a bit awkward I think without some conversion help.

The reason I'm not a fan of the conversion helpers is because it's something we'll have to come back and clean up. I think the Pass() work will be done in 1 week, at which point, we can see just how many things explode with a typedef. If fixing the explosions doesn't seem feasible, then it would be reasonable to revisit this.

An update on progress:

We've continued to make progress on the conversion from scoped_ptr to unique_ptr. The scoped_ptr::Pass() method is now ifdef'd to not exist on some platforms (linux and android at the time of this writing). Our base::Callback implementation also support supports unique_ptr now.

Once we no longer differ from unique_ptr in API (ie Pass() is really gone), we'll attempt to turn it into a typedef and rewrite all scoped_ptrs to unique_ptrs.

The styleguide owners discussed allowing std::unique_ptr today, and we decided to leave it until we're ready to convert all our scoped_ptrs to it, in order to keep things more consistent (or inconsistent for a shorter period of time).

[Dana Jansens]

I don't want to kill this thread or the idea. The styleguide/c++/OWNERS discussed this today, and since there is not yet motivation or data to support replacing RefCounted with shared_ptr, https://codereview.chromium.org/1570443005/ will ban the use of std::shared_ptr for now. We don't want to add use of yet another general ref-counting class to the codebase without any plans to remove one.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CANh-dXm3BUjPKd26SnWHpgBTW4j1P8w9LrxAVy_0MzW70iKO1w%40mail.gmail.com.

[Daniel Cheng]

On Thu, Jan 7, 2016 at 3:43 PM Dana Jansens <dan...@google.com> wrote:

On Thu, Dec 17, 2015 at 5:12 PM, Daniel Cheng <dch...@chromium.org> wrote:

On Thu, Dec 17, 2015 at 12:11 PM Dana Jansens <dan...@google.com> wrote:

On Thu, Dec 17, 2015 at 10:38 AM, Daniel Cheng <dch...@chromium.org> wrote:

I'm hoping to land https://codereview.chromium.org/1532433002 as well (for our pre C++14 version of std::make_unique).

It would also be nice to be able to seamlessly convert to/from scoped_ptr, but without VS2015, it's going to be fairly verbose so (IMO) adding a conversion helper doesn't add much. IMO, we should just encourage new code to use std::unique_ptr and work on finishing the migration away from scoped_ptr in parallel.

Being able to convert without losing the deleter (release() will drop it) would be nice.

For the record, some attempts at converting between were done here https://codereview.chromium.org/1432193002/.

Implicitly converting scoped_ptr to unique_ptr should be done with an rvalue method, but ref-qualifiers do not work in MSVC 2013: https://msdn.microsoft.com/en-us/library/hh567368.aspx#corelanguagetable.

At that time we were considering trying to make scoped_ptr a subset of unique_ptr (matching exactly except not allowing reference deleters), and then making scoped_ptr a typedef for unique_ptr. I forget where that derailed..

That's still in process: bulk conversion of Pass() to std::move() is the largest remaining task that I know of... unless we missed some other subtle difference between std::unique_ptr and scoped_ptr.

 

I think we could still maybe consider doing implicit conversion from unique_ptr to scoped_ptr, and provide a helper method to convert a scoped_ptr to unique_ptr (not a member method so that it can take a scoped_ptr&&).

I don't think there's any reason to ban using std::unique_ptr right now, but converting existing code is still going to be a bit awkward I think without some conversion help.

The reason I'm not a fan of the conversion helpers is because it's something we'll have to come back and clean up. I think the Pass() work will be done in 1 week, at which point, we can see just how many things explode with a typedef. If fixing the explosions doesn't seem feasible, then it would be reasonable to revisit this.

An update on progress:

We've continued to make progress on the conversion from scoped_ptr to unique_ptr. The scoped_ptr::Pass() method is now ifdef'd to not exist on some platforms (linux and android at the time of this writing). Our base::Callback implementation also support supports unique_ptr now.

Once we no longer differ from unique_ptr in API (ie Pass() is really gone), we'll attempt to turn it into a typedef and rewrite all scoped_ptrs to unique_ptrs.

The styleguide owners discussed allowing std::unique_ptr today, and we decided to leave it until we're ready to convert all our scoped_ptrs to it, in order to keep things more consistent (or inconsistent for a shorter period of time).

At this point, scoped_ptr is already std::unique_ptr under the hood and (almost) all the blockers for removing //base/memory/scoped_ptr.h are gone. The fixes for the remaining blockers should land today or Monday. How do the styleguide OWNERS feel about doing a wholesale conversion of scopd_ptr -> std::unique_ptr and allowing it everywhere after that?

Note that there's still an open question of whether or not we want base::MakeUnique: https://codereview.chromium.org/1532433002 I'm hoping the styleguide owners could weigh in on this as well, so we can figure out what to do with make_scoped_ptr. For the switchover though, the easiest (and probably best thing) to do is s/make_scoped_ptr/WrapUnique/.

Daniel

[Dana Jansens]

On Fri, Mar 25, 2016 at 3:03 PM, Daniel Cheng <dch...@chromium.org> wrote:

On Thu, Jan 7, 2016 at 3:43 PM Dana Jansens <dan...@google.com> wrote:

On Thu, Dec 17, 2015 at 5:12 PM, Daniel Cheng <dch...@chromium.org> wrote:

On Thu, Dec 17, 2015 at 12:11 PM Dana Jansens <dan...@google.com> wrote:

On Thu, Dec 17, 2015 at 10:38 AM, Daniel Cheng <dch...@chromium.org> wrote:

I'm hoping to land https://codereview.chromium.org/1532433002 as well (for our pre C++14 version of std::make_unique).

It would also be nice to be able to seamlessly convert to/from scoped_ptr, but without VS2015, it's going to be fairly verbose so (IMO) adding a conversion helper doesn't add much. IMO, we should just encourage new code to use std::unique_ptr and work on finishing the migration away from scoped_ptr in parallel.

Being able to convert without losing the deleter (release() will drop it) would be nice.

For the record, some attempts at converting between were done here https://codereview.chromium.org/1432193002/.

Implicitly converting scoped_ptr to unique_ptr should be done with an rvalue method, but ref-qualifiers do not work in MSVC 2013: https://msdn.microsoft.com/en-us/library/hh567368.aspx#corelanguagetable.

At that time we were considering trying to make scoped_ptr a subset of unique_ptr (matching exactly except not allowing reference deleters), and then making scoped_ptr a typedef for unique_ptr. I forget where that derailed..

That's still in process: bulk conversion of Pass() to std::move() is the largest remaining task that I know of... unless we missed some other subtle difference between std::unique_ptr and scoped_ptr.

 

I think we could still maybe consider doing implicit conversion from unique_ptr to scoped_ptr, and provide a helper method to convert a scoped_ptr to unique_ptr (not a member method so that it can take a scoped_ptr&&).

I don't think there's any reason to ban using std::unique_ptr right now, but converting existing code is still going to be a bit awkward I think without some conversion help.

The reason I'm not a fan of the conversion helpers is because it's something we'll have to come back and clean up. I think the Pass() work will be done in 1 week, at which point, we can see just how many things explode with a typedef. If fixing the explosions doesn't seem feasible, then it would be reasonable to revisit this.

An update on progress:

We've continued to make progress on the conversion from scoped_ptr to unique_ptr. The scoped_ptr::Pass() method is now ifdef'd to not exist on some platforms (linux and android at the time of this writing). Our base::Callback implementation also support supports unique_ptr now.

Once we no longer differ from unique_ptr in API (ie Pass() is really gone), we'll attempt to turn it into a typedef and rewrite all scoped_ptrs to unique_ptrs.

The styleguide owners discussed allowing std::unique_ptr today, and we decided to leave it until we're ready to convert all our scoped_ptrs to it, in order to keep things more consistent (or inconsistent for a shorter period of time).

At this point, scoped_ptr is already std::unique_ptr under the hood and (almost) all the blockers for removing //base/memory/scoped_ptr.h are gone. The fixes for the remaining blockers should land today or Monday. How do the styleguide OWNERS feel about doing a wholesale conversion of scopd_ptr -> std::unique_ptr and allowing it everywhere after that?

I look forward to it! I see no point in pretending we're using scoped_ptr anymore. Farewell sweet smart pointer, you served us well. Thanks for your tireless work in deleting everything from scoped_ptr.h, Daniel.

Note that there's still an open question of whether or not we want base::MakeUnique: https://codereview.chromium.org/1532433002 I'm hoping the styleguide owners could weigh in on this as well, so we can figure out what to do with make_scoped_ptr. For the switchover though, the easiest (and probably best thing) to do is s/make_scoped_ptr/WrapUnique/.

I still think that we want base::MakeUnique, as we're going to get std::make_unique in C++14, and I don't imagine us not using it.

Converting all make_scoped_ptr to WrapUnique sounds okay, it will be an improvement, even if MakeUnique would be better in many cases. So we can figure out MakeUnique-ness in parallel to making this transition I think.

- Dana

[Jeremy Roman]

On Fri, Mar 25, 2016 at 7:57 PM, Dana Jansens <dan...@chromium.org> wrote:

On Fri, Mar 25, 2016 at 3:03 PM, Daniel Cheng <dch...@chromium.org> wrote:

On Thu, Jan 7, 2016 at 3:43 PM Dana Jansens <dan...@google.com> wrote:

On Thu, Dec 17, 2015 at 5:12 PM, Daniel Cheng <dch...@chromium.org> wrote:

On Thu, Dec 17, 2015 at 12:11 PM Dana Jansens <dan...@google.com> wrote:

On Thu, Dec 17, 2015 at 10:38 AM, Daniel Cheng <dch...@chromium.org> wrote:

I'm hoping to land https://codereview.chromium.org/1532433002 as well (for our pre C++14 version of std::make_unique).

It would also be nice to be able to seamlessly convert to/from scoped_ptr, but without VS2015, it's going to be fairly verbose so (IMO) adding a conversion helper doesn't add much. IMO, we should just encourage new code to use std::unique_ptr and work on finishing the migration away from scoped_ptr in parallel.

Being able to convert without losing the deleter (release() will drop it) would be nice.

For the record, some attempts at converting between were done here https://codereview.chromium.org/1432193002/.

Implicitly converting scoped_ptr to unique_ptr should be done with an rvalue method, but ref-qualifiers do not work in MSVC 2013: https://msdn.microsoft.com/en-us/library/hh567368.aspx#corelanguagetable.

At that time we were considering trying to make scoped_ptr a subset of unique_ptr (matching exactly except not allowing reference deleters), and then making scoped_ptr a typedef for unique_ptr. I forget where that derailed..

That's still in process: bulk conversion of Pass() to std::move() is the largest remaining task that I know of... unless we missed some other subtle difference between std::unique_ptr and scoped_ptr.

 

I think we could still maybe consider doing implicit conversion from unique_ptr to scoped_ptr, and provide a helper method to convert a scoped_ptr to unique_ptr (not a member method so that it can take a scoped_ptr&&).

I don't think there's any reason to ban using std::unique_ptr right now, but converting existing code is still going to be a bit awkward I think without some conversion help.

The reason I'm not a fan of the conversion helpers is because it's something we'll have to come back and clean up. I think the Pass() work will be done in 1 week, at which point, we can see just how many things explode with a typedef. If fixing the explosions doesn't seem feasible, then it would be reasonable to revisit this.

An update on progress:

We've continued to make progress on the conversion from scoped_ptr to unique_ptr. The scoped_ptr::Pass() method is now ifdef'd to not exist on some platforms (linux and android at the time of this writing). Our base::Callback implementation also support supports unique_ptr now.

Once we no longer differ from unique_ptr in API (ie Pass() is really gone), we'll attempt to turn it into a typedef and rewrite all scoped_ptrs to unique_ptrs.

The styleguide owners discussed allowing std::unique_ptr today, and we decided to leave it until we're ready to convert all our scoped_ptrs to it, in order to keep things more consistent (or inconsistent for a shorter period of time).

At this point, scoped_ptr is already std::unique_ptr under the hood and (almost) all the blockers for removing //base/memory/scoped_ptr.h are gone. The fixes for the remaining blockers should land today or Monday. How do the styleguide OWNERS feel about doing a wholesale conversion of scopd_ptr -> std::unique_ptr and allowing it everywhere after that?

I look forward to it! I see no point in pretending we're using scoped_ptr anymore. Farewell sweet smart pointer, you served us well. Thanks for your tireless work in deleting everything from scoped_ptr.h, Daniel.

+1 to rewriting (even though there'll be some churn). We want to use std::unique_ptr going forward, and having two names will simply be confusing.

 

Note that there's still an open question of whether or not we want base::MakeUnique: https://codereview.chromium.org/1532433002 I'm hoping the styleguide owners could weigh in on this as well, so we can figure out what to do with make_scoped_ptr. For the switchover though, the easiest (and probably best thing) to do is s/make_scoped_ptr/WrapUnique/.

I still think that we want base::MakeUnique, as we're going to get std::make_unique in C++14, and I don't imagine us not using it.

Converting all make_scoped_ptr to WrapUnique sounds okay, it will be an improvement, even if MakeUnique would be better in many cases. So we can figure out MakeUnique-ness in parallel to making this transition I think.

I agree that this is orthogonal to rolling out std::unique_ptr. I'm not very opinionated about make_unique (I don't know that it's a huge win).

Aside: it would be nice if code search could detect (through annotations or similar) constructs like make_unique and go through to the constructor. In my experience it seems it will simply take you through to the utility function that forwards to the constructor (and you can go no further, because that is just a template). Not the end of the world, but it's nice to be able to jump to the exact constructor that is called.

[Nico Weber]

+1 on getting rid of scoped_ptr from me too. Thank you for all your work on this!