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