CODEWITHSUNDEEP
c++pointersvector
xyz
xyz

Reputation: 186

Copy constructor for a vector of pointers

I'm trying to create a node class that contains a vector of pointers. Here's my code:

node.h:

#ifndef NODE_H
#define NODE_H

class node
{
public:
    vector<node*> next;
    void add_arc(node & a)

    string some_string;

#endif

node.cpp:

void node::add_arc(node & a)
{
    node *b = &a;
    next.push_back(b);       //only copyies nodes
}

main.cpp:

int main()
{
    vector<node> nodes;
    node a;
    node b;
    node c;

    a.somestring = "a";
    b.somestring = "b";
    c.somestring = "c";



    a.add_arc(b);      //a should point to b
    a.add_arc(c);      //a should point to c

    nodes.push_back(a);
    nodes.push_back(b);
    nodes.push_back(c);

    cout << nodes[0].next.size() << endl;           // prints "2", works fine
    cout << nodes[0].next[0]->some_string << endl;  //empty
}

I thought it would be as easy as just overloading push_back:

void push_back(vertex * pointer)
{
    next.push_back(pointer);
}

But I think I really need a copy constructor, or some other method to make this work. How would I go about doing this for a vector of pointers?

Edit: I guess I didn't explain it well. Look at the answers in this question: Segmentation fault when accessing a pointer's member function in a vector Making 'a' a reference did not work for me

Upvotes: 0

Views: 2330

Answers (1)

Christophe
Christophe

Reputation: 73366

It works...

Your code generates as expected the correct output (see online demo): 

2
b

...However this design is not future proof

However this result is related somehow to luck, because in your code snippet:

  • the nodes in the nodes vector are copies of the original object including all their pointers
  • the local objects a, b, c to which these pointers point still exist

However in more complex code, you'd quickly end up with dangling pointers. Imagine:

  • Bad example 1: you create a graph, keeping all the nodes directly in a vector of nodes. You then add the first arcs between the nodes. As soon as you'll add a new node to the vector, reallocation might occur and you'd risk to see all your next pointers invalidated.
  • Bad example 2: you initialise a graph like you did, but in a function called by main. In this case, as soon as you return from this function, all the local nodes get destroyed and the vector's node will point to objects that do no longer exist. UB guaranteed !

How to improve ?

Your design fails to recognize that the nodes all belong to the same graph.

There is a quick and dirty way out: always create the node from the free store, and store them in a vector<node*>. 

vector<node*> nodes;
node *a = new node("a");  // Imagine a node constructor 
node *b = new node("b");
a->add_arc(b);            //change signature, to accept a pointer 
nodes.push_back(a);
nodes.push_back(b);

There's a better approach: improve further the previous approach, but use shared_ptr<node*> to make sure that nodes that are no longer referenced (neither by a vector of nodes, nor by an arc) are destroyed automatically.

There's an even better approach: encapsulate the nodes in a class representing a graph. In this case, you could consider using a vector<nodes> and replace the pointers in next, by indexes of the target nodes in the vector. No pointer, but perfect copy of graphs will be much easier. And no more memory management hassle.

class node    // just to give the general idea
{
public:
    vector<int> next;  // not usable without the graph 
    void add_arc(int a)
    string id;
};

class graph {
    vector<node> nodes; 
public:  
    void add_node (node a);  
    void add_arc (string from, string to);  
    node& operator[] (size_t i); 
    ...
};

Upvotes: 1

Related Questions