How to store a list of smart pointers inside a c++ class?

Question

I have a class Node that must have a list of its input Edge's. These input Edge's, however, are not meant to be modified by Node, only accessed. I have been advised to use smart pointers for that matter, like this:

class Node
{
private:
    std::vector<std::unique_ptr<Edge>> inEdges;
//...
public:
    void inline AddEdge(std::unique_ptr<Edge>& edge) // could be const here too
    {
        this->inEdges.push_back(edge);
    }
//...
}

So in the runtime I can create a list of nodes and edges and assign edges to each node:

int main()
{
    std::vector<std::unique_ptr<Nodes>> nodes;
    std::vector<std::unique_ptr<Edges>> edges;
    nodes.push_back(std::make_unique<Node>(0, 0.5, 0.0));
    nodes.push_back(std::make_unique<Node>(1, 0.5, 0.0));
    edges.push_back(std::make_unique<Edge>(*nodes[0], *nodes[1], 1.0));
    nodes[1]->AddEdge(edges[0]);
}

The compiler gives the error

Error 1 error C2280: 'std::unique_ptr>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)' : attempting to reference a deleted function c:\program files (x86)\microsoft visual studio 12.0\vc\include\xmemory0 593 1 sirs

It used to work with raw pointers inside the Node's std::vector<Edge*>, given that the signature of AddEdge would be void AddEdge(Edge& edge);, pushing back &edge inside the vector.

What is wrong with my code? How should I proceed to correct it? Given that std::vector cannot store references, because these are not assignable.

PS: I do not want to transfer ownership of the pointers to the Node objects...

Answer 1

You should only move instances of std::unique_ptr in order to get them in a std::vector. Otherwise you need std::shared_ptr.

class Node
{
private:
    std::vector<std::unique_ptr<Edge>> inEdges;
//...
public:
    void AddEdge(std::unique_ptr<Edge>&& edge)
    {
        inEdges.push_back(std::move(edge));
    }
//...
}

I've changed AddEdge to take an r-value reference in order to support move semantics properly.

To call:

node.AddEdge(std::move(edge));

Or:

node.AddEdge(std::make_unique<Edge>(/*args*/));

As an aside. If you do find you're passing std::unique_ptr by reference, it might be a sign you should be using std::shared_ptr.

Also, inline is redundant on methods declared inside a class.

Answer 2

std::vector::push_back copies its argument, and std::unique_ptr can't be copied, it can only be moved.

You'll have to move the passed std::unique_ptr:

void inline AddEdge(std::unique_ptr<Edge> edge)
{
    this->inEdges.push_back(std::move(edge));
}

Then call it like so:

nodes[1]->AddEdge(std::move(edges[0]));

---

Also, inEdges doesn't have to be a vector for std::unique_ptr, as they should be non-owning. A std::unique_ptr implies ownership, but as you said, Node doesn't own the Edges.

Use std::vector<Edge*> instead (or std::observer_ptr when it is implemented in the near future), as raw pointers (normally) imply non-ownership. You just have to be careful that the lifetime of the std::vector<Edge*> doesn't exceed the lifetime of the std::unique_ptrs, or else the pointers will be dangling.

Answer 3

I would not use smart pointers at all for this. I think the simplest route is to store all your nodes and edges as values in the main vectors and then use raw pointers to express their relationships.

You don't need smart pointers because the vectors manage the lifetime and destruction of your nodes and edges so nothing else needs to own them.

The most important thing to remember with this scheme is that you must never change the size of a vector after the data structure that uses pointers to its elements has been created.

The reason being that changing the size of a vector invalidates all the pointers (the objects may be moved to different memory locations).

So something like this:

class Edge
{
    class Node* a;
    class Node* b;
    double w;

public:
    Edge(Node* a, Node* b, double w): a(a), b(b), w(w) {}
};

class Node
{
private:
    double x, y, z;
    std::vector<Edge*> inEdges;

public:
    Node(double x, double y, double z): x(x), y(y), z(z) {}

    void AddEdge(Edge* edge)
    {
        this->inEdges.push_back(edge);
    }
};

int main()
{
    std::vector<Node> nodes; // values, not pointers
    std::vector<Edge> edges;

    // NOTE: You MUST create ALL of the Nodes BEFORE
    // adding their pointers to the Edges and you must
    // create ALL the Edges BEFORE adding their pointers
    // to the Nodes because resizing the vectors will
    // INVALIDATE all the pointers

    // FIRST create ALL the nodes
    nodes.emplace_back(0, 0.5, 0.0); // make nodes[0]
    nodes.emplace_back(1, 0.5, 0.0); // make nodes[1]

    // NEXT create ALL the edges
    edges.emplace_back(&nodes[0], &nodes[1], 1.0); // make edges[0]

    // Finally add the edges to the nodes
    nodes[1].AddEdge(&edges[0]);

}

Answer 4

You are trying to push a copy of the std::unique_ptr into the vector. The vector is attempting to create a copy of the unique_ptr, which isn't allowed since it would defeat the purpose of unique ownership.

You should first consider which object owns which object, and only use std::unique_ptr if there is indeed a case for unique ownership.

There are a couple things you can do:

1. std::move() the pointer into the vector, which will cause the previous copy to become invalid (see: move semantics).
2. Use std::shared_ptr if you want multiple objects to hold copies of the pointer. Since it looks like you want to have multiple copies of the pointer, this is probably the direction you want to go.

Its probably worth while in your case to read up on std::weak_ptr since I think you may want objects to refer to one another, without creating circular references. Weak pointers work on shared_ptr's without actually incrementing the reference count, and are important in some cases where you want to make sure objects aren't hanging onto references too long and creating memory leaks.

Hope this helps.