Vector of Deep Copy of pointers

Question

I am a very new programmer and a super beginner so I don't know too much about c++. I had specifically a question regarding making deep copies of pointers. What I have is a class A full of POD's and a pointer to this class (A *P). I have a second class B which contains some other POD's and a vector of pointers to Class A. I want to fill this vector of deep copies of A *P because in a loop I will be dynamically allocating and deallocating it. The following does not work. I believe its my copy constructor and overloading of the = operator. This is something I am doing for fun and learning.

class A
{
   public:  
   .....
   .....
   .....
};

class B
{
   public:  
   B();
  ~B();
   B(const B &Copier);
   B& B::operator=(const B &Overloading);
   vector<A*> My_Container;
   A* Points_a_lot;
   int counter;
 };
B::B()
{
  counter=0;
  Points_a_lot=NULL;
}
B::~B()
{
   for(size_t i=0; i<My_Container.size(); i++)
   {
      delete My_Container[i];
    }
 }
 B::B(const B &Overloading)
 {
     My_Container[counter]=new A(*Overloading.Points_a_lot);
 }
 B& B::operator=(const B &Overloading)
 {
     if(!Overloading.My_Container.empty()) 
     {
         Overloading.My_Container[counter]=new B(*Overloading.Points_a_lot);
      }
      return *this; 
  } 
 int main()
 {  A* p=NULL;
    B Alphabet;
    for(....)
    {
        p=new A;
        //some stuff example p->Member_of_A=3; etc..
        Alphabet.My_Container[Alphabet.counter]=p;
        Alphabet.counter++;
       delete p;
     }
    return 0;
   }

Any help will be great. I thank you for your time. Assume needed libraries included.

Answer 1

There are many errors in your code. The main one is that your assignment operator and copy constructor are not deep copying a vector of pointers to A at all, rather, you are trying to put a B* in a location of the vector. What your assignment operator should do is to delete the elements the vector points to, and fill it with deep copies of the elements the source object's vector points to, after checking for self assignment. Your copy constructor should be filled with deep copies of the elements of the source objects.

Second, you should provide a method that adds elements to your class' vector, and let it set the counter variable internally. Having to coordinate both the vector and the counter externally is error-prone and one of the advantages of OOP is to avoid that kind of error. But better still, remove the counter variable altogether. You don't need it. YOur main would then be simplified to this:

int main()
{
  B Alphabet;
  for(....)
  {
    A* p = new A;
    //some stuff example p->Member_of_A=3; etc..
    Alphabet.appendElement(p); // B takes ownership, no need to delete in main
  }
}

and appendElement could be

class B
{
 public:
  void appendElement(A* element) { myContainer_.push_back(element); }
  // other public methods
 private:
   std::vector<A*> myContainer_;
};

You could further improve all of this by storing some kind of single ownership smart pointer instead of raw pointers. That would mean you don't have to worry about making deletions yourself. But that is probably beyond the scope of this question.

Now, you should consider avoiding the pointers altogether. In this case, you have to provide no copy constructors, assignment operators or destructors. The compiler-synthesized ones will do just fine. Your class B reduces to

class B
{
 public:
  void appendElement(const A& element) { myContainer_.push_back(element); }
  // other public methods
 private:
   std::vector<A> myContainer_;
};

Answer 2

You may want to take a look at boost::ptr_vector. Its interface is very similar to that of a std::vector but it is tailored for pointers and thus:

• owns the resources (so no memory leak)
• allow polymorphic storage (derived classes)
• is const-correct and copy-correct

In order for the copy to work, you'll have to provide a A* new_clone(A const*) implementation.

Answer 3

I do not fully understand what your requirements are so I attempted to fix the code and do a deep copy of B as that is what it seems you are asking.

#include <vector>
using namespace std;

class A
{
public:
    A() : m_someInt(0), m_someFloat(0.0f) {}

    // Redundant but putting it here for you to see when it is called (put break-point)
    A(const A& a_other)
    {
        m_someInt = a_other.m_someInt;
        m_someFloat = a_other.m_someFloat;
    }

    int   m_someInt;
    float m_someFloat;
};

class B
{
public:  
    B();
    ~B();
    B(const B &Copier);
    B& B::operator=(const B &Overloading);
    void Cleanup();
    void AddA(const A* a);

private:

    vector<A*> My_Container;
    A* Points_a_lot;
};

B::B()
{
    Points_a_lot=NULL;
}

B::~B()
{
    Cleanup();
}

B::B(const B &Overloading)
{
    // Deep copy B
    operator=(Overloading);
}

B& B::operator=(const B &Overloading)
{
    // Delete old A's
    Cleanup();

    // Not using iterators to keep it simple for a beginner
    for (size_t i = 0; i < Overloading.My_Container.size(); ++i)
    {
        // We need new A's which will then copy from the A's in Overloading's container
        A* newA = new A( *(Overloading.My_Container[i]) );
        // Done with allocation and copy, push_back to our container
        My_Container.push_back(newA);
    }

    return *this; 
}

void B::Cleanup()
{
    // Assuming B is not responsible for cleaning up Points_a_lot
    Points_a_lot = NULL;

    for (size_t i = 0; i < My_Container.size(); ++i)
    {
        delete My_Container[i];
    }

    // Automatically called when My_Container is destroyed, but here we 
    // are open to the possibiliy of Cleanup() being called by the client
    My_Container.clear(); 
}

void B::AddA(const A* a)
{
    // We are adding a new A. In your code, the incoming A is going to 
    // be destroyed, therefore, we need to allocate a new A and copy 
    // the incoming A
    A* newA = new A(*a);
    My_Container.push_back(newA);
}

int main()
{  
    A* p=NULL;
    B Alphabet;
    for(int i = 0; i < 10; ++i)
    {
        p = new A();
        //some stuff example p->Member_of_A=3; etc..
        Alphabet.AddA(p);
        delete p;
    }

    // If you put a breakpoint here and step through your code, you will see
    // `B` deep-copied
    B NewAlphabet = Alphabet;

    return 0;
}

A few notes:

• Newing and deleteing A in the loop is a bad idea. I realize you are doing this just to learn, which is great, but you may want to keep this in mind. Instead of destroying A through p, allow B to take ownership of the new A
• Step through the code by using a debugger to see how it works
• Try to post code that is as close to your original as possible when asking "why doesn't this work/compile"

Answer 4

Looks like you should instead have your My_Container consist of unique_ptrs so that when you assign, that the vector takes over ownership of the A instance

for(....)
{
    p=new A;
    //some stuff example p->Member_of_A=3; etc..
    Alphabet.My_Container[Alphabet.counter]=p;
    Alphabet.counter++;
   delete p;
 }

so instead declare My_Container as

vector<std::unique_ptr<A*> > My_Container;

then the code will be

for(....)
{
    p=new A;
    //some stuff example p->Member_of_A=3; etc..
    Alphabet.My_Container[Alphabet.counter]=p;
    Alphabet.counter++;
 }

then if you need to do a 'deep copy' create a member in A called clone() or something and return a unique_ptr to the instance, use that when you need to create a copy.

Answer 5

Okay, it seems to me that you are very confused about what operator= should be doing. Take a look at this page on operator overloading. This should get you started down the right path for that function.

Second, unrelated to your question check out this question about why your fields (member-variables, what-have-you) should be private.