Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

Am I over-using shared_ptr?

97 views
Skip to first unread message

wthol...@gmail.com

unread,
Oct 20, 2015, 1:52:22 PM10/20/15
to
I'm concerned that I'm over-using shared_ptr. Here's a simplified version of my object model, which I think is sufficient for the purpose of this discussion.


// A simplified object model.

// A Patch is a collection of Nodes
class Patch {

public:

void AddNode(const shared_ptr<Node>&);
void RemoveNode(const shared_ptr<Node>&);

private:

vector< shared_ptr<Node> > _nodes;

};

// A node knows which patch it belongs to, and has
// connections to other nodes.
class Node {

public:

Node(const shared_ptr<Patch>& patch) : _patch(patch) { }
Connect(const shared_ptr<Node>&);

private:

weak_ptr<Patch> _patch;
vector< weak_ptr<Node> > _connections;

};

This design is nice because it always avoids dangling references. However, the client gets to control the lifetime of a `Node` which can be problematic in other parts of my code (specifically, undo).

If I understand the C++ cognoscenti correctly, they would recommend avoiding `shared_ptr`, preferring to use `unique_ptr` since `Patch` effectively *owns* the `Nodes`:

// A Patch is a collection of Nodes
class Patch {

public:

// Patch takes ownership of the node
void AddNode(unique_ptr<Node>);
void RemoveNode(Node*);

private:

vector< unique_ptr<Node> > _nodes;

};

// A node knows which patch it belongs to, and has
// connections to other nodes.
class Node {

public:

Node(const shared_ptr<Patch>& patch) : _patch(patch) { }
Connect(Node*);

private:

weak_ptr<Patch> _patch;
vector< Node* > _connections;

};

This design is nice because the lifetime of a Node is easy to understand: it's deallocated when `RemoveNode` is called. However, the client could be left with a dangling pointer. Granted, this would be bad behavior for the client in the first place, but I'd like such errors to be easier to catch. Also, catching errors in `Node::_connections` is harder.

**Now, before I go and write some sort of fancy handle or intrusive weak pointer, is there an easier way to avoid the client controlling the lifetime while still being safe?**

Thanks!!

Alf P. Steinbach

unread,
Oct 20, 2015, 6:25:16 PM10/20/15
to
On 10/20/2015 7:52 PM, wthol...@gmail.com wrote:
> I'm concerned that I'm over-using shared_ptr. Here's a simplified version of my object model, which I think is sufficient for the purpose of this discussion.
>
>
> // A simplified object model.
>
> // A Patch is a collection of Nodes
> class Patch {
>
> public:
>
> void AddNode(const shared_ptr<Node>&);
> void RemoveNode(const shared_ptr<Node>&);
>
> private:
>
> vector< shared_ptr<Node> > _nodes;
>
> };
>
> // A node knows which patch it belongs to, and has
> // connections to other nodes.
> class Node {
>
> public:
>
> Node(const shared_ptr<Patch>& patch) : _patch(patch) { }
> Connect(const shared_ptr<Node>&);
>
> private:
>
> weak_ptr<Patch> _patch;
> vector< weak_ptr<Node> > _connections;
>
> };
>
> This design is nice because it always avoids dangling references. However,
> the client gets to control the lifetime of a `Node` which can be problematic
> in other parts of my code (specifically, undo).

You should ask whether a Node needs to belong to a Patch in order to be
useful. If so then this design allows too much.


> If I understand the C++ cognoscenti correctly, they would recommend avoiding
> `shared_ptr`, preferring to use `unique_ptr` since `Patch` effectively
> *owns* the `Nodes`:

Yes, that sounds more like it.

Or, even better, just raw pointers, except in the formal argument to
AddNode.


> // A Patch is a collection of Nodes
> class Patch {
>
> public:
>
> // Patch takes ownership of the node
> void AddNode(unique_ptr<Node>);
> void RemoveNode(Node*);
>
> private:
>
> vector< unique_ptr<Node> > _nodes;
>
> };
>
> // A node knows which patch it belongs to, and has
> // connections to other nodes.
> class Node {
>
> public:
>
> Node(const shared_ptr<Patch>& patch) : _patch(patch) { }
> Connect(Node*);
>
> private:
>
> weak_ptr<Patch> _patch;
> vector< Node* > _connections;
>
> };
>
> This design is nice because the lifetime of a Node is easy to understand: it's
> deallocated when `RemoveNode` is called.

The weak_ptr says that an owning Path may not exist.

Is that really the case?



> However, the client could be left with a dangling pointer.

How so? Aren't all connected nodes in the same patch?

If they're not, then instead of `vector<Node*>` you may use a
``vector<shared_ptr<Node>>`, and use the /aliasing constructor/ of
shared_ptr to create a shared_ptr with ownership of a patch and
referencing a node owned by that patch.

For that approach you'd need to add a patch argument to the Connect method.


> Granted, this would be bad behavior for the client in the
> first place, but I'd like such errors to be easier to catch. Also, catching
> errors in `Node::_connections` is harder.
>
> **Now, before I go and write some sort of fancy handle or intrusive weak
> pointer, is there an easier way to avoid the client controlling the lifetime
> while still being safe?**

Dunno, but I think there's a sort of smell about passing unique_ptr to
Node to a patch which takes ownership. Why not let a patch provide the
node storage. Then it can just have a std::list of nodes.

That is, using collection instead of smart pointer.


Cheers & hth.,

- Alf


wthol...@gmail.com

unread,
Oct 20, 2015, 7:05:18 PM10/20/15
to
On Tuesday, October 20, 2015 at 3:25:16 PM UTC-7, Alf P. Steinbach wrote:
No. Good point. That was to break the cycle between patches and nodes in the first implementation.

>
>
>
> > However, the client could be left with a dangling pointer.
>
> How so? Aren't all connected nodes in the same patch?

something like:

auto node = make_uniqe<MyAwesomeNodeSubclass>();
auto nodePtr = node.get();
patch->AddNode(node);
patch->RemoveNode(nodePtr);

// nodePtr now dangles

Maybe that interface is pretty lame anyway. Feels dirty to have
to grab the raw pointer out of the unique_ptr.

They're all in the same patch. Adding a node to multiple patches
should trigger an error.

>
> If they're not, then instead of `vector<Node*>` you may use a
> ``vector<shared_ptr<Node>>`, and use the /aliasing constructor/ of
> shared_ptr to create a shared_ptr with ownership of a patch and
> referencing a node owned by that patch.
>
> For that approach you'd need to add a patch argument to the Connect method.
>
>
> > Granted, this would be bad behavior for the client in the
> > first place, but I'd like such errors to be easier to catch. Also, catching
> > errors in `Node::_connections` is harder.
> >
> > **Now, before I go and write some sort of fancy handle or intrusive weak
> > pointer, is there an easier way to avoid the client controlling the lifetime
> > while still being safe?**
>
> Dunno, but I think there's a sort of smell about passing unique_ptr to
> Node to a patch which takes ownership. Why not let a patch provide the
> node storage. Then it can just have a std::list of nodes.
>
> That is, using collection instead of smart pointer.

Because Node is actually an abstract base class.

>
>
> Cheers & hth.,
>
> - Alf

Thanks for your help Alf!

Christopher Pisz

unread,
Oct 20, 2015, 7:49:38 PM10/20/15
to
There isn't enough information.

One should look at the following in a design mindset rather than an
implementation mindset:

What is a node, exactly?
What is a patch exactly?
Who creates a node?
Who creates a patch?
Who owns a node?
Who owns a patch?
Does anyone else ever have to call any methods or manipulate data on a node?
Does anyone else ever have to call any methods or manipulate data on a
patch?
What's the lifetime of a node?
What's the lifetime of a patch?

Answer all those questions being completely oblivious to whether you
will be passing by value, reference, pointer, or what kind of pointer,
but instead decide what you'll be using based on the answers.












--
I have chosen to troll filter/ignore all subthreads containing the
words: "Rick C. Hodgins", "Flibble", and "Islam"
So, I won't be able to see or respond to any such messages
---

wthol...@gmail.com

unread,
Oct 20, 2015, 8:21:10 PM10/20/15
to
On Tuesday, October 20, 2015 at 4:49:38 PM UTC-7, Christopher Pisz wrote:
Ok, see http://docs.audulus.com for some background.

>
> What is a node, exactly?

A unit of signal processing.

> What is a patch exactly?

A collection of nodes and connections between the nodes.
(I've simplified the OM somewhat, there are also Inputs and Outputs)

> Who creates a node?

The user, via a command. Also, tests.

> Who creates a patch?

The user, by creating a new document. Also, tests.

> Who owns a node?

The patch.

> Who owns a patch?

The client code.

> Does anyone else ever have to call any methods or manipulate data on a node?

Yes, the patch calls various methods on the nodes for drawing and handling user events. Tests call various methods.

> Does anyone else ever have to call any methods or manipulate data on a
> patch?

The client code calls methods on the patch for drawing and handling user events. Also, tests call various methods.

> What's the lifetime of a node?

From when the user creates it or loads the patch, to when the user deletes it, or closes the patch.

> What's the lifetime of a patch?

It's essentially a document.

>
> Answer all those questions being completely oblivious to whether you
> will be passing by value, reference, pointer, or what kind of pointer,
> but instead decide what you'll be using based on the answers.
>
>

Does that help?

Nobody

unread,
Oct 20, 2015, 9:17:55 PM10/20/15
to
On Tue, 20 Oct 2015 17:20:35 -0700, wtholliday wrote:

>> Who owns a node?
>
> The patch.

>> What's the lifetime of a node?
>
> From when the user creates it or loads the patch, to when the user deletes
> it, or closes the patch.
>
>> What's the lifetime of a patch?
>
> It's essentially a document.

This tends to suggest that it should probably be a unique_ptr rather than
a shared_ptr.

The patch owns the node. The node can't be created before the patch which
owns it. Destroying the patch will destroy the node (if it still exists).

shared_ptr is for the case when ownership isn't clear cut; when the object
needs to exist for as long as *something* references it and (ideally) no
longer than that, but there's no easy way of determining what could
reference it.

tay...@audulus.com

unread,
Oct 21, 2015, 11:25:35 PM10/21/15
to
Cool. Makes sense.

So what would you recommend for ensuring clients of this API can't create
dangling pointers? For example, I were to wrap this out to Python, I'd
have to ensure the client can't create a dangling pointer.

Richard

unread,
Oct 22, 2015, 1:11:28 PM10/22/15
to
[Please do not mail me a copy of your followup]

tay...@audulus.com spake the secret code
<4996a24c-a188-4fe2...@googlegroups.com> thusly:

>So what would you recommend for ensuring clients of this API can't create
>dangling pointers? For example, I were to wrap this out to Python, I'd
>have to ensure the client can't create a dangling pointer.

In my opinion, you can guard against some things, but people who are
determined to write code that is abusive can't be stopped. Strive for
the former and don't waste any brain cycles on the latter.

There's a bunch of stuff in the new C++ Core Guidelines being proposed
by Bjarne Stroustrup and Herb Sutter with contributions from all over
the community on how to guard against lifetime errors. Some of this
may help you, but when exposing C++ to a garbage collected language
like python, it may not be as relevant.

<https://github.com/isocpp/CppCoreGuidelines>

CppCon 2015: Bjarne Stroustrup "Writing Good C++14"
<https://www.youtube.com/watch?v=1OEu9C51K2A>

CppCon 2015: Herb Sutter "Writing Good C++14... By Default"
<https://www.youtube.com/watch?v=hEx5DNLWGgA>
--
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
The Computer Graphics Museum <http://computergraphicsmuseum.org>
The Terminals Wiki <http://terminals.classiccmp.org>
Legalize Adulthood! (my blog) <http://legalizeadulthood.wordpress.com>

asetof...@gmail.com

unread,
Oct 22, 2015, 4:54:21 PM10/22/15
to
Richard wrote:
><https://github.com/isocpp/CppCoreGuidelines>

The problems he speak are already
solved for me, in some code, using
one malloc allocator that allow
check for leaks at end of
program, and one free function
that see if some write
is in the 2 borders of memory
returned (and see if that pointer
is right to be free)
I not use in the C++
new or delete
nor I understand well them

tay...@audulus.com

unread,
Oct 22, 2015, 5:25:56 PM10/22/15
to
On Thursday, October 22, 2015 at 10:11:28 AM UTC-7, Richard wrote:
>
> In my opinion, you can guard against some things, but people who are
> determined to write code that is abusive can't be stopped. Strive for
> the former and don't waste any brain cycles on the latter.

Well, if a user can crash my app because they misused the scripting API,
then that's a bug. That's why I'd like the code to be safe.

>
> There's a bunch of stuff in the new C++ Core Guidelines being proposed
> by Bjarne Stroustrup and Herb Sutter with contributions from all over
> the community on how to guard against lifetime errors. Some of this
> may help you, but when exposing C++ to a garbage collected language
> like python, it may not be as relevant.
>
> <https://github.com/isocpp/CppCoreGuidelines>
>
> CppCon 2015: Bjarne Stroustrup "Writing Good C++14"
> <https://www.youtube.com/watch?v=1OEu9C51K2A>
>
> CppCon 2015: Herb Sutter "Writing Good C++14... By Default"
> <https://www.youtube.com/watch?v=hEx5DNLWGgA>

I appreciate you mentioning that. I've been watching those videos intensely the
past few days :)

Richard

unread,
Oct 22, 2015, 8:04:57 PM10/22/15
to
[Please do not mail me a copy of your followup]

tay...@audulus.com spake the secret code
<373de084-1f56-4bcf...@googlegroups.com> thusly:

>On Thursday, October 22, 2015 at 10:11:28 AM UTC-7, Richard wrote:
>> In my opinion, you can guard against some things, but people who are
>> determined to write code that is abusive can't be stopped. Strive for
>> the former and don't waste any brain cycles on the latter.
>
>Well, if a user can crash my app because they misused the scripting API,
>then that's a bug. That's why I'd like the code to be safe.

Agreed. I agree that taking reasonable precautions are worthwhile.
I like the idea of template decorations that compile away but reveal
your intention around the usage of a raw pointer. Can this pointer be
nullptr? Who is the owner of this raw pointer? These are reasonable
things to guard against and I like the idea of the support library
defining these helpers that cost you nothing at runtime but allow
tools to identify mismatched intentions. This is really cool stuff.

However, I've been in code reviews where people get themselves all
into a contorted knot because they are imaginging programmers that are
either evil or are doing things that are frankly stupid. Things like
"but if I return a reference to a const object from a function, what if
they store the address of the reference?".

This is not a reasonable precuation. This is supposing that the other
members of your team are either outright idiots or are intentionally
writing malicious code. Worrying about this stuff is pointless.

There are some "Guru of the Week" questions show ways in which you can
intentionally subvert the type system (void *, anyone?) or do even
more malicious things like sneaking your way past the published class
declaration and subtituting your own so that you can poke into private
member variables. This is the sort of stuff that is abnormal, abusive
or idiotic. Trying to guard against this is not only unreasonable, it
is impossible. Without a virtual machine to play traffic cop, you
have access to all the bytes in the program's address space and can do
whatever malicious things you want.

woodb...@gmail.com

unread,
Oct 22, 2015, 11:36:44 PM10/22/15
to
Any recommendations? I've watched a few of them. Andrei's
talk on allocators was interesting.

https://duckduckgo.com/?q=andrei+allocators++vector&ia=videos&iai=LIb3L4vKZ7U

Brian
Ebenezer Enterprises - In G-d we trust.
http://webEbenezer.net

Mr Flibble

unread,
Oct 23, 2015, 11:17:31 AM10/23/15
to
Hi,

You can also use shared_ptr in a non-owning mode which will allow you to
have shared_ptrs to both nodes that patches own and nodes with some
other ownership (e.g. a node on the stack). I do something similar in
my C++ GUI library for widget ownership:

http://neogfx.org

For more information see the shared_ptr aliasing constructor ((8) in
http://en.cppreference.com/w/cpp/memory/shared_ptr/shared_ptr)

HTH.

/Leigh

Nobody

unread,
Oct 23, 2015, 9:59:41 PM10/23/15
to
On Wed, 21 Oct 2015 20:24:45 -0700, taylor wrote:

> So what would you recommend for ensuring clients of this API can't create
> dangling pointers? For example, I were to wrap this out to Python, I'd
> have to ensure the client can't create a dangling pointer.

Dangling pointers aren't the issue there; the issue is ensuring that
clients don't create garbage, i.e. create nodes (which are owned by the
patch) then forget about them.

If the patch is only owning the nodes on behalf of its clients, then
preventing this issue comes down to ensuring that there's a one-to-one
correspondence between a node and a Python object representing the node.
Creating such an object creates a node and its destructor deletes it.
Python's own GC will manage the lifecycle of the object (i.e. the
destructor won't get called so long as something holds a reference to it).

But if there are nodes which aren't managed by the client, then you
would probably need shared ownership, or you'll end up with an awkward
API. If the patch owns the nodes, anything else can only borrow a
reference temporarily, so you can't have e.g. Python methods returning
references to nodes (as those could be stored indefinitely).

asetof...@gmail.com

unread,
Oct 24, 2015, 3:45:27 AM10/24/15
to
I made one error link...
This would be the right one:
C++14
<https://www.youtube.com/watch?v=1OEu9C51K2A>
0 new messages