Vectors and classes

Question

I have the following two classes: the first one

class node{
public:
   node(); //default constructor 
   node(const node&);//copy constructor
   ~node(); //destructor
   node& operator= (const node&);

   short visits;
   bool inizpc;
   cards cards_first;
   cards cards_second;
   cards * first; //cards is another class, don't mind that
   cards * second;
   int * suits;
   node * children;
    //other members
}

and the class

class tree{
public:
    tree() : //initialization list {branch.reserve(20)};//constructor
    tree(const tree&); //copy constructor
    ~tree();//destructor
    tree& operator= (const tree&);

    std::vector<node> branch;
    //other members

    void tries(node&);
}

My problem arise when i try to use the branch vector, in particular when i use the .push_back() function inside the following function

 void tree::tries(node& a){
    a.visits++;
    branch.push_back(a);
 }

the compiler just terminate the function without running it. The problem is the branch.push_back but I've not used vectors much so I don't get where the error stand and what i'm missing. Also, how does push_back allocate the object i pass? Does it call the copy constructor?

EDIT: It seems my problem is in the destructor

~node(){
 cout << "deleting node\n";
 delete[] first;
 delete[] second;
 delete[] suits;
 delete[] children;
 }

I tried doing something like

node a;
a.visits=1;
cout << a.visits;

commenting out the destructor definition and declaration it works fine. Is there a reason for it to be deleted before execution ends?

also, here the default constructor, copy constructor and copy assignement definitions, the destructor is as defined above

//default constructor
node::node():
                    visits(0),
                    inizpc(false)
                    cards_first(0,0);//cards have a parameter ctor
                    cards_second(0,0);
{
    first=new carte[10];
    second=new carte[10];
    suits=new short[4];
    children=new node[visits];
}


//copy constructor
node::node(node& a){
    visits=a.visits;
    inizpc=a.inizpc;
    cards_first=a.cards_first;
    cards_second=a.cards_second;

    first=new cards[10];
    for(int i=0; i<10; i++) first[i]=a.first[i];

    second=new cards[10];
    for (int i=0; i<10; i++) second[i]=a.second[i];

    suits=new int[4];
    for(int i=0; i<4; i++) suits[i]=a.suits[i];

    children=new node[a.visits];
    for (int i=0; i<a.visits; i++) children[i]=a.children[i];
}


//assignement
node& node::operator= (node& a){
    visits=a.visits;
    inizpc=a.inizpc;
    cards_first=a.cards_first;
    cards_second=a.cards_second;

    delete[] first;
    first=new cards[10];
    for(int i=0; i<10; i++) first[i]=a.first[i];

    delete[] second;
    second=new cards[10];
    for (int i=0; i<10; i++) second[i]=a.second[i];

    delete[] suits;
    suits=new int[4];
    for(int i=0; i<4; i++) suits[i]=a.suits[i];

    delete[] children;
    children=new node[a.visits];
    for (int i=0; i<a.visits; i++) children[i]=a.children[i];
}

Answer 1

Given that you posted your copy constructor, destructor, your assignment operator was not implemented. Instead you posted something that looked like a combination of a copy constructor and an assignment operator.

In your original post, you really did not implement an assignment operator. I don't know if you hastily posted your class and just plain forgot to post it, but in any event, you need this function to complete the rule of 3.

As my comment also suggested, a lack of an assignment operator for a class such as node will bite you at some point, either now or later -- you can't avoid it.

To correctly write the assignment operator, using the copy / swap idiom is the easiest thing to do, given you have a copy constructor and destructor that seem to be doing things correctly:

#include <algorithm>

node& operator=(const node& a)
{
    node temp(a);
    std::swap(temp.visits, visits);
    std::swap(temp.inizpc, initzpc);
    std::swap(temp.cards_first, cards_first);
    std::swap(temp.cards_second, cards_second);
    std::swap(temp.first, first);
    std::swap(temp.second, second);
    std::swap(temp.suits, suits);
    std::swap(temp.children, children);
    return *this;
}

All we did was create a temporary, swapped out the internals of the current object with the temporary, and let the temporary die at the end with the old data. Also note that a reference is returned.

This technique only works if the copy constructor and destructor are flawless, and that the copy constructor does not use the assignment operator as a helper function (as unfortunately, a few C++ programmers seem to insist on doing). Again, glancing at your code, it seems these two functions were written correctly, so copy / swap for the assignment operator would be the best way to complete the job.

After you do this, you should thoroughly test the node class in a simple main program. You don't need std::vector to test whether your class has correct copy semantics:

int main()
{
   node n1;
   // assume that n1 has been populated with some data
   //...
   node n2 = n1;
   node n3;
   n3 = n1;
}

That one small main function tests copy construction, assignment, and destruction. If there is anything faulty, a program such as above will be faulty. The program above should be able to be run from beginning to end with no memory errors, no memory leaks, no double-deallocations, etc.

In addition, your tree class does not need a destructor, assignment operator, or copy constructor. You should remove them, since tree has all members that are safely copyable (the node class can now be copied without error, given that it would now have correct copy semantics).