Creating an easy to maintain copy constructor

Question

Hi,

Consider the following class:

class A {

char *p;
int a, b, c, d;

public:
   A(const &A);
};

In the above I have to define a copy constructor in order to do a deep copy of "p". This has two issues:

1. most of the fields should simply copied. Copying them one by one is ugly and error prone.

2. the more important problem is that whenever a new attribute is added to the class copy constructor should also be updated and this creates a maintenance nightmare.

I personally like to do something in theory like:

A(const A &a) : A(a)
{
// do deep copy of p
.
.
.
}

So the default copy constrictor is called first and then the deep copy is performed but unfortunately this doesn't seem to work. Is there any better way to do this (I can't use shared/smart pointers.)

But unfortunately this doesn't work. Do you have a better solution?

Thanks in advance!

Answer 1

Replace char* with std::string.

Answer 2

Always use RAII objects to manage unmanages resources such as raw pointers, and use exactly one RAII object for each resource. Avoid raw pointers in general. In this case, using std::string is the best solution.

If that's not possible for some reason, factor the easy to copy parts out into a base class or a member object.

Answer 3

You really should use smart pointers here.

This would avoid rewriting both the copy constructor and the affectation operator (operator=).

Both of these are error prone.

A common mistake with the operator= is implementing it that way:

SomeClass& operator=(const SomeClass& b)
{
  delete this->pointer;
  this->pointer = new char(*b.pointer); // What if &b == this or if new throws ?

  return *this;
}

Which fails when one does:

SomeClass a;
a = a; // This will crash :)

Smart pointers already handle those cases and are obviously less error prone.

Moreover, Smart pointers, like boost::shared_ptr can even handle a custom deallocation function (by default it uses delete). In practice, I rarely faced a situation where using a smart pointer instead of a raw pointer was unpractical.

Just a quick note: boost smart pointer class, are header-only designed (based on templates) so they don't require additional dependencies. (Sometimes, it matters) You can just include them and everything should be fine.

Answer 4

The question is, do you really need a pointer with deep-copy semantics in your class? In my experience, the answer almost always is no. Maybe you could explain your scenario, so we may show you alternative solutions.

That said, this article describes an implementation of a smart-pointer with deep-copy semantics.

Answer 5

You could separate your copyable members into a POD-struct and mantain your members requiring a managed copy separately.

As your data members are private this can be invisible to clients of your class.

E.g.

class A {

char *p;

struct POData {
    int a, b, c, d;
    // other copyable members
} data;

public:
   A(const &A);
};

A(const A& a)
    : data( a.data )
{
    p = DuplicateString( a.p );
    // other managed copies...
    // careful exception safe implementation, etc.
}

Answer 6

While I agree with others saying that you should wrap the pointer in its own class for RAII and let the compiler synthesise the copy contructor, destructor and assignment operator there is a way around your problem: declare (and define) private static function which will do whatever is needed and common for different constructors and call it from there.

Answer 7

So the default copy constrictor is called first and then the deep copy is performed but unfortunately this doesn't seem to work. Is there any better way to do this (I can't use shared/smart pointers.)

If I understand correctly, your question, you could consider using an initialization function:

class A
{
    int i, j;
    char* p;

    void Copy(int ii, int jj, char* pp); // assign the values to memebers of A
public:
    A(int i, int j, char* p);
    A(const A& a);
};

A::A(int i, int j, char* p)
{
    Copy(i, j, p);
}

A::A(const A& a)
{
    Copy(a.i, a.j, a.p);
}

That said, you really should consider using RAII ( there's a reason people keep recommending it :) ) for your extra resources.

If I can't use RAII, I still prefer creating the copy constructor and using initializer lists, for every member (actually, I prefer doing so even when using RAII):

A::A(int ii, int lj, char* pp)
    : i(ii)
    , j(jj)
    , p( function_that_creates_deep_copy(pp) )
{
}

A::A(const A& a)
    : i(a.i)
    , j(a.j)
    , p( function_that_creates_deep_copy(a.p) )
{
}

This has the advantage of "explicitness" and is easy to debug (you can step in and see what it does for each initialization).

Answer 8

Unless your class has one function, which is managing a resource, you should never manage any resources directly. Always use a smart pointer or custom management class of some description. Typically, it's best to leave the implicit copy constructor, if you can. This approach also allows easy maintenance of the destructor and assignment operators.

Answer 9

Sbi's suggestions make a lot of sense. I think I'll go with creating wrapper classes for handling the resource. I don't want to user shared_ptr since boost libraries may not be available on all platforms (at least not in standard distributions, OpenSolaris is an example).

I still think it would have been great if you could somehow make the compiler to create the default constructor/assignment operators for you and you could just add your functionality on top of it. The manually created copy constructor/assignment operator functions I think will be a hassle to create and a nightmare to maintain. So my personal rule of thumb would be to avoid custom copy constructors/assignment operators at all cost.

Thanks everybody for their responses and helpful information and sorry about typos in my question. I was typing it from my phone.

Regards,

Roux

Answer 10

@Tomek: The point is to avoid writing the code that the compiler would have automatically created for you and just add you custom "deep copy" code to it. Creating one/multiple static functions really doesn't solve the problem since you still have to create a copy constructor that calls those static functions.