Problem with copy constructor

[bijuabrahamp]

I was just experimenting with copy constructors and I got stuck by an odd output.
I wrote a program that uses classes as arrays (safe arrays).

#include <iostream>
#include <cstdlib>
using namespace std;

class array{
	int *p;
	int size;
public:
	array(int sz) {
		p = new int[sz];
		cout << "Using normal constructor" << endl;
		size = sz;
		if(!p) exit(1);
	}
	~array() { cout << "Destructing.. " << endl; delete [] p; }

	array(const array &a) {
		p = new int[a.size];

		for(int i=0; i<size; i++) p[i] = a.p[i];							
		size = a.size;
		cout << "Using copy constructor" << endl << endl;
	}
	int &getar(int x) {
		if(x<size && x>=0)	return p[x]; 
		else {
			cout << "Array size invalid : " << x << endl;
			exit(1);
		}
	}
};

int main()
{
	array x(10);
	for(int i=0; i<10; i++)	
		x.getar(i) = i*2;
	array y = x;
	cout << "array y's data : ";
	for(i=0; i<10; i++)
		cout << y.getar(i) << ' ';
	cout << endl << endl;	
	cout << "array x's data : ";
	for(i=0; i<10; i++)
		cout << x.getar(i) << ' ';
	cout << endl;
	return 0;
}

The output I get is very strange :

Using normal constructor
Using copy constructor

array y's data : -842150451 -842150451 -842150451 -842150451 -842150451 -842150451 -842150451 -842150451 -842150451 -842150451

array x's data : 0 2 4 6 8 10 12 14 16 18
Destructing..
Destructing..

I expected array y and array x to have the same data.. but the output shows a different result.. can somebody tell why this happens?

[Dave McLelland]

your copy constructor has an error.

you are copying the data from the array being copied but using the "size" field from the array receiving the copy. Put the size = a.size before the loop.
The size variable is not initialised before it is used.
Hope this helps.

array(const array &a) {
		p = new int[a.size];

		size = a.size;

		for(int i=0; i<size; i++) p[i] = a.p[i];							
		cout << "Using copy constructor" << endl << endl;

[bijuabrahamp]

Oh.. i was careless there.. thanks

[Paul McKenzie]

Oh.. i was careless there.. thanks

Also, you have no user-defined assignment operator. It makes no sense to have a copy constructor without an assignment operator.

int main()
{
    array x(10);
    array y(5);
    x = y;  // what happens here?
    x = x;  // what happens here?
} // what happens at the destructor?

Also, hopefully this is an exercise, since std::vector<int> does all of this work you are trying to code.

Regards,

Paul McKenzie

[bijuabrahamp]

I was just learning how to use copy constructor. Anyway, thanks for making me aware about this.

[rene1]

of course it makes sense to have a copy-constructor without
assigment:in the case you have function-calls by value!

[exterminator]

of course it makes sense to have a copy-constructor without
assigment:in the case you have function-calls by value!

Wh are you trying to say here? If function calls are made by value then of course you need the copy constructor. And regarding the assignment operator overload .. that is very necessary - in all cases where you think you should write you own copy constructor (i havent found an exception to this yet). Example - suppose you have a pointer member in your class that is to point to some dynamically allocated block of memory and hence you would rightly think that you should write your own copy constructor so that a deep copy of any object of this class of yours happens (the pointer points to a copy of that member and not to the old memory block). Now, suppose you dont overload the assignment operator then in case you do any assignment then you would be losing memory ( as you are not freeing it while the assignment is being done and the initial object is being abandoned by the assignment with some new object). Hope this clarifies a bit. Let me know if I confused you somewhere..

[Paul McKenzie]

of course it makes sense to have a copy-constructor without
assigment:in the case you have function-calls by value!

Please look up the "rule of three" for C++.

If you code a destructor, you should code assignment operator and copy constructor. Also, to someone looking at the code, it doesn't make sense to be able to do this:

Foo f1 = f2;

and not be able to do this:

Foo f1;
Foo f2;
f1 = f2;

By only coding a copy constructor, you are going against how every class or simple type works. Every class in the standard library or simple type (int, double, etc.) can be assigned and created from an existing type. Also it is just natural to the person using a class that assigning and copying are analogous, and one of these operations shouldn't be able to be done without the other.

Regards,

Paul McKenzie

[NMTop40]

Here is how it should be done.

Create a member function swap(). If you don't want it called externally make it private. Thus:

void array::swap( array & a )
{
   int tempsize = a.size;
   a.size = size;
   size = tempsize;

   int* tempp = a.p;
   a.p = p;
   p = tempp;
}

Now you might implement assignment thus:

array& array::operator=( const array& a )
{
    array acopy( a );
    swap( acopy );
    return *this;
}

Of course you should not be coding all this at all, because there is std::vector. And there is std::swap() template function to take care of the member swaps, thus you can do

std::swap( size, a.size );
std::swap( p, a.p );

2 lines of code instead of 6.

There may be good reason not to implement assignment with swap, and that is because you are committed to allocating a new buffer, whereas your buffer may be adequate in size already. So you might want to do it this way instead:

array& array::operator=( const array & a )
{
   if ( size >= a.size )
   {
        size = a.size;
        for ( int i=0; i<size; ++i )
       {
            p[i] = a.p[i];
       }
   }
   else
   {
       array acopy( a );
       swap( acopy );
   }
   return *this;
}

You also might want 2 size values, one logical size and another the size of your buffer, called capacity. capacity is always >= size. It can be an advantage to have it greater than size so you do not need to keep reallocating more memory if you are growing.

std::vector provides a function reserve() for this purpose, and in addition will usually allocate more than necessary on a call to extend the size by one element (push_back()).

Note that STL also provides a copy algorithm so once again the for loop in my code could be replaced by

std::copy( a.p, a.p + size, p );

which actually usually expands (inline) to

void copy( const int * begin, const int * end, int * dest )
{
     while ( begin != end )
     {
          *dest = *begin;
          ++dest;
          ++begin;
     }
}

and that can actually be faster than the for-loop using "pointer+integer" arithmetic.
Note it can also expand to this:

void copy( const int * begin, const int * end, int * dest )
{
     while ( begin != end )
     {
          *dest++ = *begin++;
     }
}

although in my opinion this is a worse implementation. It looks like 2 lines of code fewer but in reality the post-increment can be a lot more expensive than the pre-increment in the case where you have iterators that are not pointers.