9

I have this problem, there is a function foo() as follows,

vector<ClassA> vec;

void foo()
{
    ClassA a;   //inside foo, a ClassA object will be created
    a._ptr = new char[10];

    vec.push_back(a);   //and this newly created ClassA object should be put into vec for later use
}

And AFAIK, vec will invoke ClassA's copy-ctor to make a copy of the newly created object a, and here is the problem. If I define ClassA's copy-ctor the usual way,

ClassA::ClassA(const ClassA &ra) : _ptr(0)
{
    _ptr = ra._ptr;
}

then object a and its copy (created by vec) will have pointers _ptr pointing to the same area, when foo finishes, a will call the destructor to release _ptr, then a's copy in vec will be a dangling pointer, right? Due to this problem, I want to implement ClassA's copy-ctor this way,

ClassA::ClassA(ClassA &ra) : _ptr(0) //take non-const reference as parameter
{
    std::swap(_ptr, a._ptr);
}

Is my implementation ok? Or any other way can help accomplish the job?

Bo Persson
  • 90,663
  • 31
  • 146
  • 203
Alcott
  • 17,905
  • 32
  • 116
  • 173

5 Answers5

10

To answer your titular question: Yes, any constructor for a class T that has one mandatory argument of type T & or T const & (it may also have further, defaulted arguments) is a copy constructor. In C++11, there's also a move constructor which requires one argument of type T &&.

Having a non-constant copy constructor that actually mutates the argument gives your class very unusual semantics (usually "transfer semantics") and should be extensively documented; it also prevents you from copying something constant (obviously). The old std::auto_ptr<T> does exactly that.

If at all possible, the new C++11-style mutable rvalue references and move constructors provide a far better solution for the problem of "moving" resources around when they're no longer needed in the original object. This is because an rvalue reference is a reference to a mutable object, but it can only bind to "safe" expressions such as temporaries or things that you have explicitly cast (via std::move) and thus marked as disposable.

Kerrek SB
  • 464,522
  • 92
  • 875
  • 1,084
4

C++11 introduced move constructors for this exact purpose:

ClassA::ClassA(ClassA&& ra)
: _ptr(ra._ptr)
{
    ra._ptr = nullptr;
}

Alternatively you can declare _ptr to be a shared pointer:

std::shared_ptr<char[]> _ptr;

and then default denerated copy constructor will do just fine.

yuri kilochek
  • 12,709
  • 2
  • 32
  • 59
  • 2
    Pathetically, I'm not using C++11 – Alcott Jul 27 '12 at 08:20
  • 1
    `shared_array`, not `shared_ptr` :) – tumdum Jul 27 '12 at 08:20
  • @timdum shared_array is from boost, in std the is only shared_ptr, but i is specialised for arrays. – yuri kilochek Jul 27 '12 at 08:21
  • @Alcott: Even without C++11 you can still get many of the advantages of move constructors (and move semantics in general) through [Boost.Move](http://www.boost.org/doc/libs/release/doc/html/move.html). – Mankarse Jul 27 '12 at 08:30
  • Disregard my previous comment, std::shared_ptr [is not specialised](http://stackoverflow.com/questions/8947579/why-isnt-there-a-stdshared-ptrt-specialisation), deleter has to be provided explicitly on construction – yuri kilochek Jul 27 '12 at 08:33
2

You should not copy the pointer, you should copy the context that the pointer is pointing to. You should also initialize the class by telling it how many elements you want, instead of allocating it by accessing a public pointer.

Since you want to copy the object, not move it, you should allocate resources in the new object when copying.

class A {

        int* p_;
        int size_;
public:

        A(int size) 
        : p_(new int[size]()), 
        size_(size) {
        }

        A(const A &a) 
        : p_(new int[a.size_]),
         size_(a.size_) {               
                std::copy(a.p_, a.p_ + a.size_, p_);
        }

        ...

};


int main () {
        A a(10);
        vec.push_back(a);
}

However, if you know that the object you will copy isn't used after it's copied, you could move it's resources instead.

Man of One Way
  • 3,904
  • 1
  • 26
  • 41
  • But he wants to NULL the pointer in the original object, eg change owner – Bakery Jul 27 '12 at 08:44
  • 3
    He says "if I define the copy ctor the usual way". That's not the usual way of defining a copy ctor. It's not mentioned that he wants to move it, so I'm assuming he wants a correct copy ctor. – Man of One Way Jul 27 '12 at 08:46
  • Yes but he also continues to explain why that wont do and it is because of the deep copy required, which is the problem that you address here. If you read more and try to interpret the code that he writes you will see that he is trying to swap the pointers from the original class to the new class, and thus setting the original pointer to NULL and the new copy:s pointer to the old pointers address. That's why he don't want to use the standard copy constructor that takes a const argument. – Bakery Jul 27 '12 at 08:52
  • @Bakery Well that's what move constructors are for. Copy constructors must NOT invalidate the old object, and resources that shouldn't be shared between objects should still be separate after calling the copy constructor. – Paul Stelian Apr 16 '17 at 17:19
1

The problem with your implementation is that you will not be able to pass temporary objects as arguments for this copy-ctor (temporaries are always const). Like already mentioned the best solution would be to move to c++11 and use move semantics. If it is not possible shared_array can be an alternative.

tumdum
  • 1,981
  • 14
  • 19
0

Additional comment:

Avoid these kind of problems creating the object with new and storing pointers to the object in the vector.

steffen
  • 8,572
  • 11
  • 52
  • 90
  • 3
    I don't think trying to solve memory management issues with more explicit memory management is a good idea. Dynamically allocating objects just to work around a broken copy constructor is a bad advice. – Luc Touraille Jul 27 '12 at 10:58