How to clone objects containing pointers?

Question

I have a problem. I need to clone objects class containing pointers. An example of the problem is in the following code:

#include "stdafx.h"
#include <iostream>
#include <string.h>
#include <vector>

class CPoint
{
protected:
    int m_x;
    int m_y;

    int *m_p;

public:
    CPoint();
    CPoint(int x, int y);
    ~CPoint();

    CPoint*         clone();
    static CPoint*  clone(CPoint& p);

    int getX();
    int getY();
    void setX(int x);
    void setY(int y);

    void toString();
};

int CPoint::getX()
{
    return m_x;
}

int CPoint::getY()
{
    return m_y;
}

void CPoint::setX( int x )
{
    m_x = x;
}

void CPoint::setY( int y )
{
    m_y = y;
}

void CPoint::toString()
{
    std::cout << "(" << m_x << ", " << m_y<< ", " << *m_p << ")" << std::endl;
}

CPoint::CPoint( int x, int y )
{
    m_x = x;
    m_y = y;    

    m_p = new int();
    *m_p = x + y;
}

CPoint::CPoint()
{
    m_p = new int();
    *m_p = 1000;
}

CPoint* CPoint::clone()
{
    CPoint *p = new CPoint();
    *p = *this;
    return p;
}

CPoint* CPoint::clone( CPoint& p )
{
    CPoint *q = new CPoint();
    *q = p;
    return q;
}

CPoint::~CPoint()
{
    if (m_p) {
        delete m_p;
        m_p = NULL;
    }
}

int _tmain(int argc, _TCHAR* argv[])
{
    CPoint *p1 = new CPoint(10, 20);
    CPoint *p2 = new CPoint(30, 40);

    p1->toString();
    p2->toString();

    CPoint *p3;
    p3 = CPoint::clone(*p1);

    p3->toString();

    CPoint *p4;
    p4 = p2->clone();

    p4->toString();

    p1->setX(50);
    p1->setY(60);
    p2->setX(80);
    p2->setY(90);

    p3->toString();
    p4->toString();

    delete p1;
    delete p2;
    delete p3;
    delete p4;

    int a;
    std::cin >> a;

    return 0;
}

The problem I have with the variable m_p. When clone objects p1 and p2 on p3 and p4, the memory addresses p1 and p3 are different but m_p address is the same. Obviously, when remove p1, p3 removal fails. With p2 and p4 is the same.

How I can clone a CPoint class object?

Answer 1

You seem to be applying the rules of some other Java like language to C++.
This is a fundamental problem and going to lead to all sorts of problems in the long run.

You need to learn the idioms of C++.

In C++ you want to use C++ strings (std::string) not the C-String interface.

#include <string.h>   // C-Interface

// What you really want
#include <string>     // C++ Interface

If your class contains a pointer then you are probably doing something wrong. RAW pointers should be wrapped in a smart pointer (or containers) to control their lifespan correctly. If you put a pointer into an business class you are breaking the separation of concerns principle.

class CPoint
{
    protected:
        int m_x;
        int m_y;

        int *m_p;  // What is it supposed to be?
                   // Who owns it?

Since your class had a pointer it broke the rule of three.
If you wanted to manage the pointer in this class (and you don't (breaking separation of concerns)) then you should have implemented the rule of three (rule of five in C++11) (look it up). If you want to learn how handle a RAW pointer look here https://stackoverflow.com/a/1846409/14065

There is no need for a clone method. This is what the copy constructor is for. You are not writing a class that needs to be cloned (otherwise it would have had a virutal destructor). Your class is not polymorphic and will not be derived from. Thus a copy constructor will work perfectly.

CPoint*         clone();
static CPoint*  clone(CPoint& p);

// Copy constructor looks like this:
CPoint(CPoint const& rjs)

// Assignment operator looks like this:
CPoint& operator=(CPoint& rhs)

But non of this is required if correctly wrap your RAW pointer in an appropriate class. The compiler generated default versions of these methods will work fine.

Good way to completely destroy encapsulation.

int getX();
int getY();
void setX(int x);
void setY(int y);

To string! Poop. What you really want is a serialization method.

void toString();

// serializer look like this:

friend std::ostream& operator<<(std::ostream& stream, CPoint const& data)
{
     // Convert CPoint (data) to the stream.
     return stream;
}

In C++ we do not dynamically create objects unless we need to.
And here you do not need to. Creating local objects works better because their lifespan is guaranteed even in the presence of exceptions.

// Rather than dynamically creating them
CPoint *p1 = new CPoint(10, 20);
CPoint *p2 = new CPoint(30, 40);

// Just declare two local variables:
CPoint  p1 = CPoint(10, 20);
CPoint  p2(30, 40);           // Alternative to the above but means the same.

// Much better to use operator<<
// Also shows the functions are badly named. You are not converting to string.
// but rather printing them to a stream.
p1->toString();
p2->toString();

std::cout << p1;
myFileStream << p2;  // allows you to easily specify the actual stream.

Copy constructor work much better for copying an object

CPoint *p3;
p3 = CPoint::clone(*p1);

// If we were still using pointers. 
CPoint* p3 = new CPoint(p1);

// But much nicer to not even use pointers
CPoint  p3(p1);

Its usually a design mistake if you ever see manual call to delete in a function.

delete p1;
delete p2;
delete p3;
delete p4;

If you have pointers wrapping them in smart pointers (or container) like classes makes them exception safe to use. This is because for local objects the destructor is guaranteed to be called and thus your object will correctly deleted the pointer when it goes out of scope. Currently this code is not exception safe and will leak if an exception propagates passed them.

Small note: main() is special. If you don't specify a return value the compiler plants return 0; for you. If your application has no error state best to use this functionality as a sign to other developer that your code will always exit cleanly.

return 0;

I would re-write like this:

#include <iostream>
#include <string>
#include <vector>

class CPoint
{
protected:
    int m_x;
    int m_y;

    std::vector<int> m_p;

public:
    // If you don't explicitly initialize m_x and m_y them
    // they will have indeterminate (random) values.
    CPoint()             : m_x(0), m_y(0) {m_p.push_back(1000);}
    CPoint(int x, int y) : m_x(x), m_y(y) {m_p.push_back(x + y);}

    int getX()        { return m_x;}
    int getY()        { return m_y;}
    void setX(int x)  { m_x = x;}
    void setY(int y)  { m_y = y;}

    friend std::ostream& operator<<(std::ostream& stream, CPoint const& d)
    {
        return stream << "(" << d.m_x << ", " << d.m_y<< ", " << d.m_p[0] << ")" << std::endl;
    }
};




int main(int argc, char* argv[])
{
    CPoint p1(10, 20);
    CPoint p2(30, 40);

    std::cout << p1 << p2;

    CPoint p3(p1);

    std::cout << p3;

    CPoint p4(p2);
    std::cout << p4;

    p1.setX(50);
    p1.setY(60);
    p2.setX(80);
    p2.setY(90);

    std::cout << p1 << p2 << p3 << p4;
    int a;
    std::cin >> a;
}

Answer 2

Assuming that m_p points to only one integer (and not a whole array), cloning can be done like this:

CPoint* CPoint::clone()
{
    CPoint* cloned = new CPoint(m_x, m_y);
    if (m_p)
    {
        cloned->m_p = new int;
        *cloned->m_p = *m_p;
    }
    return cloned;
}

Note that such a member pointer has the sole purpose of adding the additional possibility of having a NULL-value - which can have a separate meaning.

Note also that the following has to be done to avoid memory leaks and heap corruption:

• The copy constructor and assignment operator have to be "disabled" (declared private)
• The destructor must delete m_p.

Answer 3

In addition to shallow-copying the immediate data members m_x and m_y, you need to deep-copy the pointer member m_p. Since you haven't shown the constructor for this class or what m_p really points to, I'm going to assume that m_p points to the first element of an array of int. Deep-copying this entails:

1. Instantiate a new array of int that is the same (or larger) size as the original array
2. Copy each element from the original array to the new array
3. Set m_p in the cloned object to point to the first element of this new array

An example of how this might be done:

CPoint* CPoint::clone(CPoint& rhs)
{
  CPoint* ret = new CPoint;
  ret->m_x = rhs.m_x;
  ret->m_y = rhs.m_y;

  size_t m_p_count = /* somehow determine the size of rhs.m_p */;
  ret->m_p = new int[m_p_count];
  std::copy(&rhs.m_p[0], &rhs.m_p[m_p_count], ret->m_p);

  return ret;
}

A few notes about your code:

1. You would be better off using a vector<int> instead of a raw pointer to an array of int.
2. Barring #1, you should be using smart pointers instead of raw pointers
3. I don't see any way in the code above to determine the size of the array. This would be easy if you used a vector<int> -- just call vecctor<int>::size(). You need to know the size of the array in order to make a copy of it, obviously.
4. A clone() type function is generally only useful when making a copy of a polymorphic object via a base class pointer. Since your class, and your useage of it, does not fall in to this category, a clone() function is not the right way to go in the first place. Consider using a copy constructor and a copy assignment operator instead, and don't forget to also implement a destructor. Better still, avoid all of this stuff altogether and follow the Rule of Zero.

Answer 4

In this example is an integer but may be any type. The question I had was like to clone an object that contains a pointer to another type.

I believe that there are basically two situations here: you want the containing object to own the pointed-to object; or you don't want the containing object to own the pointed-to object.

Let's start with non-owning. What's the tool that C++ provides to represent non-owning pointers? Well, regular pointers are non-owning. And how do you copy a regular pointer? You do nothing. You let the compiler deal with it, generating the correct copy constructor that you can use at will (and while you're at it, let the compiler generate a destructor as well).

And what about owning? What's the tool for owning pointers? Well, for most cases you don't even need a pointer for that: just store a value directly and, again, let the compiler generate the correct copy constructor (and a destructor too!). In the example provided int m_p; would work nicely.

There is an annoyance in this situation when polymorphic base classes are involved: copying may cause slicing. Does C++ provide a tool for this situation? Sadly, it doesn't. You have to write it by hand. But do yourself a favour and don't mix these concerns with the rest of the class (Single Responsibility Principle).

Write a reusable class (bonus points: make it a template) that owns a single pointer, cleans it up on destruction, and performs a polymorphic copy (a common idiom involves a virtual clone function) in the copy constructor. Then put a value of that reusable class in your CPoint and... you guessed it! Let the compiler generate the correct copy constructor.

Answer 5

You have to ask yourself: Does each instance of your object "own" the object it points to, or do they all refer to a common object owned by something else?

When you have a case of ownership, each instance must point to an individual copy. That means that you don't have to copy the pointer, you have to create a clone of the object it points to and assign this new object to the pointer of the copy.

Answer 6

You must reallocate memory for all pointers within CPoint and copy their data into new memory. In your case you have to do following operation:

CPoint clone()
{
   CPoint p;
   p = *this;
   p.m_p = new int();
   *p.m_p = *m_p;
   return p;
}