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

1696 views
Skip to first unread message

Matthew Dempsky

unread,
Nov 12, 2015, 3:29:51 PM11/12/15
to c...@chromium.org
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

unread,
Nov 12, 2015, 3:32:10 PM11/12/15
to Matthew Dempsky, c...@chromium.org
+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

unread,
Nov 12, 2015, 4:15:58 PM11/12/15
to Ryan Hamilton, Matthew Dempsky, c...@chromium.org
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.

Daniel Cheng

unread,
Nov 12, 2015, 5:08:12 PM11/12/15
to Ryan Hamilton, Matthew Dempsky, c...@chromium.org
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

unread,
Nov 12, 2015, 5:21:20 PM11/12/15
to Daniel Cheng, Ryan Hamilton, Matthew Dempsky, cxx
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.

 

Nico Weber

unread,
Nov 12, 2015, 5:24:54 PM11/12/15
to Daniel Cheng, Ryan Hamilton, Matthew Dempsky, cxx
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:

Jeffrey Yasskin

unread,
Nov 12, 2015, 5:28:43 PM11/12/15
to Nico Weber, Daniel Cheng, Ryan Hamilton, Matthew Dempsky, cxx
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

unread,
Nov 12, 2015, 5:36:07 PM11/12/15
to Jeffrey Yasskin, Nico Weber, Daniel Cheng, Ryan Hamilton, Matthew Dempsky, cxx
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.
 

Vladimir Levin

unread,
Nov 12, 2015, 6:07:13 PM11/12/15
to Dana Jansens, Jeffrey Yasskin, Nico Weber, Daniel Cheng, Ryan Hamilton, Matthew Dempsky, cxx
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.

Matthew Dempsky

unread,
Nov 12, 2015, 6:08:27 PM11/12/15
to Dana Jansens, Jeffrey Yasskin, Nico Weber, Daniel Cheng, Ryan Hamilton, cxx
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

unread,
Nov 12, 2015, 6:13:57 PM11/12/15
to Matthew Dempsky, Dana Jansens, Jeffrey Yasskin, Daniel Cheng, Ryan Hamilton, cxx
I think we should have a single guideline for stuff in src.git, else things will get super confusing.

Matthew Dempsky

unread,
Nov 12, 2015, 6:15:09 PM11/12/15
to Nico Weber, Dana Jansens, Jeffrey Yasskin, Daniel Cheng, Ryan Hamilton, cxx
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

unread,
Nov 12, 2015, 6:16:51 PM11/12/15
to Matthew Dempsky, Nico Weber, Dana Jansens, Jeffrey Yasskin, Ryan Hamilton, cxx
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

unread,
Nov 12, 2015, 6:23:00 PM11/12/15
to Daniel Cheng, Matthew Dempsky, Dana Jansens, Jeffrey Yasskin, Ryan Hamilton, cxx
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

unread,
Nov 12, 2015, 6:52:33 PM11/12/15
to Nico Weber, Daniel Cheng, Ryan Hamilton, Matthew Dempsky, cxx
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

unread,
Nov 12, 2015, 7:07:19 PM11/12/15
to Daniel Cheng, cxx, Brett Wilson
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

unread,
Nov 12, 2015, 7:12:02 PM11/12/15
to Brett Wilson, Daniel Cheng, Matthew Dempsky, Dana Jansens, Jeffrey Yasskin, Nico Weber, Ryan Hamilton, cxx
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

unread,
Nov 12, 2015, 7:13:55 PM11/12/15
to Peter Kasting, Daniel Cheng, cxx
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

unread,
Nov 12, 2015, 7:20:32 PM11/12/15
to Daniel Cheng, Matthew Dempsky, Nico Weber, Jeffrey Yasskin, Ryan Hamilton, cxx
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

unread,
Nov 12, 2015, 8:49:56 PM11/12/15
to Daniel Cheng, Ryan Hamilton, Matthew Dempsky, cxx
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.)

James Robinson

unread,
Nov 12, 2015, 11:55:49 PM11/12/15
to Nico Weber, Daniel Cheng, Ryan Hamilton, Matthew Dempsky, cxx
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

unread,
Nov 13, 2015, 2:11:05 AM11/13/15
to Brett Wilson, Peter Kasting, Daniel Cheng, cxx
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.

Daniel Cheng

unread,
Nov 13, 2015, 6:22:21 PM11/13/15
to James Robinson, Nico Weber, Ryan Hamilton, Matthew Dempsky, cxx
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

unread,
Nov 13, 2015, 6:33:31 PM11/13/15
to Daniel Cheng, James Robinson, Nico Weber, Ryan Hamilton, Matthew Dempsky, cxx
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.

Chris Blume

unread,
Nov 18, 2015, 9:23:55 AM11/18/15
to Vladimir Levin, Brett Wilson, Peter Kasting, Daniel Cheng, cxx
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

unread,
Nov 18, 2015, 1:59:51 PM11/18/15
to Chris Blume, Brett Wilson, Peter Kasting, Daniel Cheng, cxx
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

unread,
Nov 18, 2015, 2:04:30 PM11/18/15
to Vladimir Levin, Chris Blume, Brett Wilson, Peter Kasting, Daniel Cheng, cxx
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

unread,
Nov 18, 2015, 2:06:36 PM11/18/15
to Ryan Sleevi, Vladimir Levin, Chris Blume, Peter Kasting, Daniel Cheng, cxx
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

unread,
Nov 18, 2015, 2:20:18 PM11/18/15