CODEWITHSUNDEEP
c++copy-constructor
CodeTrek
CodeTrek

Reputation: 475

My copy constructor fails.. how to copy a pointer to an object

I try to write a copy constructor for my vector of pointers to object initialized and declared in class Shop. The vector in consideration is:

std::vector <gCustomer*>   vCustomer;

It has been also declared in the constructor of gShop and deleted via loop in the destructor.

Now I want to have a deep copy of the vector of pointers in the copy constructor. But nothing gets actually copied, a check of its size remains zero or crashes the program if I manage to run the program and access vCustomer. (Note if I leave the copy constructor out so that the default copy constructor is used, the program runs fine)

gShop::gShop(const gShop & cShop)
    {
    for(int i = 0; i < (int)vCustomer.size(); ++i)
        {
        vCustomer[i]  = cShop.vCustomer[i];
        }
    }

Thanks

Note I also have an assigned operator 

gShop gShop::operator=(const gShop & rhs)
   {
   if (this == &rhs) return *this;


    for(int i = 0; i < (int)vCustomer.size(); ++i)
        {
        delete vcustomer[i];
        vCustomer[i] = new gCustomer;
        vCustomer[i]= rhs.vCustomer[i];
        }
    }

Upvotes: 0

Views: 208

Answers (3)

john
john

Reputation: 87944

You've implemented your copy constructor and assignment operator wrongly, they are doing shallow copies not deep copies, and they don't resize the target vector. Here's a deep copy copy constructor

gShop::gShop(const gShop & cShop)
{
for(int i = 0; i < (int)cShop.vCustomer.size(); ++i)
    {
    if (cShop.vCustomer[i])
        vCustomer.push_back(new gCustomer(*cShop.vCustomer[i]));
    else
        vCustomer.push_back(NULL);
    }
}

and here's a deep copy assignment operator

gShop& gShop::operator=(const gShop & rhs)
{
if (this == &rhs) return *this;
// clear any existing data
for(int i = 0; i < (int)vCustomer.size(); ++i)
    delete vcustomer[i];
vcustomer.clear();
// add the new data
for(int i = 0; i < (int)rhs.vCustomer.size(); ++i)
    {
    if (rhs.vCustomer[i])
        vCustomer.push_back(new gCustomer(*rhs.vCustomer[i]));
    else
        vCustomer.push_back(NULL);
    }
return *this;
}

Basically the problem was that the you were copying pointers instead of allocating new memory. If you want a deep copy it's essential you allocate new memory.

Of course there is the bigger question, why are you using a vector of pointers at all. One of the big advantage of a vector is that you no longer have to explicitly manage memory, by using a vector of pointers you have lost that benefit. I don't know you program but it seems to me that std::vector<gCustomer> would be better than std::vector<gCustomer*>. With std::vector<gCustomer> you don't need to write a copy constructor or assignment operator, the deep copy will happen automatically (assuming gCustomer does a deep copy).

Upvotes: 1

Zhongzhi
Zhongzhi

Reputation: 436

std::vector<> has copy constructor itself to do the deep copy. So the copy constructor the compiler gives will do the work you want actually. If you want to implement it yourself, it's better in the init list of the constructor of gShop.

gShop::gShop(const gShop & cShop):vCustomer(cShop.vCustomer)
{
}

The size of vCustomer is always zero in your copy ctor.

I'm confused with your operator=(). What do you want? I suggest you read some text books about like c++ primer.

Upvotes: -1

urzeit
urzeit

Reputation: 2909

The loop

gShop::gShop(const gShop & cShop)
    {
    for(int i = 0; i < (int)vCustomer.size(); ++i)
        {
        vCustomer[i]  = cShop.vCustomer[i];
        }
    }

uses the wrong limit. It should run from 0 to the length of the existing object:

i < (int)cShop.vCustomer.size()

Upvotes: 1

Related Questions