Pointer Malfunction

[GarroteYou]

Hello,

I'm building a simple array application, purely for my own personal edu purposes. The exercise is to overload the assignment operator.

I'm having problems with either Array::operator [], or Array::operator =. In the operator = function, the memory addresses are being set equal to the correct values:

0x002f2aa8 = 0;
0x002f2aac = 1;
0x002f2ab0 = 4;

The problem is when the [] operator is used after an assignment. The vales in the above mem addresses have been lost, and replaced with seemingly random values. Can anyone perhaps point out some things I might look into changing?

Here is the code:

#include <iostream>
using namespace std;
////////////////////////////////////////////////////////////////
class Array                     //models a normal C++ array
{
public:
	Array(int s)              //one-argument constructor
	{
		size = s;              //argument is size of Array
		ptr = new int[s];      //make space for Array
	}
	
	~Array()                  //destructor
		{ delete[] ptr; }
	
	int& operator [] (int j)  //overloaded subscript operator
	{ 
			return *(ptr+j); 
	}
	
	Array operator = ( Array& a )
	{
		if ( size != a.size)
			exit(1);

		for( int i=0; i<size; i++ )
			*(this->ptr+i) = a[i];

		return *this;
	}

private:
	int* ptr;                 //pointer to Array contents
	int size;                 //size of Array
};

////////////////////////////////////////////////////////////////
int main()
{
	const int ASIZE = 10;        //size of array
	Array arr(ASIZE);            //make an array

	for(int j=0; j<ASIZE; j++)   //fill it with squares
		arr[j] = j*j;

	Array arr2(ASIZE);
	
	arr2 = arr;

//	for(j=0; j<ASIZE; j++)       //display its contents
//		cout << arr[j] << ' ';

//	cout << "\n\n";

	for(j=0; j<ASIZE; j++)       //display its contents
		cout << arr2[j] << ' ';

	cout << endl;
	return 0;
}

[kuphryn]

The overloaded = overload skips bytes.

*(this->ptr++)

Kuphryn

[mdmd]

If you do this

Array operator = ( Array& a )
return *this

You're going to create a temporary using the default
copy constructor which when deleted will delete this->ptr.
I think this should return Array&. ?? Promote proper chaining.

Array &operator = ( const Array& a )
return *this


Really you should be supplying a copy constructor too especially
since you are supplying an operator=.
Pointer values like this should suggest a copy ctor.

[GarroteYou]

Thanks for the replies,

I changed the function to the following:

	Array &operator = ( Array& a )
	{
		if ( size != a.size)
			exit(1);

		int store=0;

		for( int i=0; i<size; i++ )
		{
			store = a[i];
			*(this->ptr+i) = store;
		}
                                
                return *this;

	}

The program outputs properly:

0 1 4 9 16 25 36 49 64 81

mdmd = your solution works, thanks. The second part of the exercise is to also create an overloaded copy constructor.

Just so I'm clear, when returning by reference, the default copy constructor is not used. When returning the object in full, a temporary object is created, the default copy constructor is used, and the destructor for the temp object deletes the ptr.

For consistency I should overload the copy constructor to do basically the same thing as the assignment operator.

kuphryn: I'm not sure I understand you, if I add an 'int' to ptr, it should move the address by the width of an 'int', which is what I need. (Right?) If I use ++, won't the compiler automatically advance the pointer by the width of the pointer type which is indeed 'int'?

[mdmd]

1) Just so I'm clear, when returning by reference, the default copy constructor is not used. When returning the object in full, a temporary object is created, the default copy constructor is used, and the destructor for the temp object deletes the ptr.

2) For consistency I should overload the copy constructor to do basically the same thing as the assignment operator.

1) Yes, return by value in this case would cause construction and
destruction of a temporary. Returning by reference would not.

2) Basically yes. But remember you already have an object in
operator=, you don't in the copy constructor so there will be
differences. Self assignment being one, current values being
another.

[Graham]

Rule of three: if you need any one of copy ctor, copy assign or dtor, then you probably need all three.

A convenient implementation of copy assign can be done quite simply if the copy ctor is available, and you supply a non-throwing swap() member function:

void foo::swap(foo& other) throw()
{
    // swap data members between this and other.
}
foo& foo::operator=(const foo& rhs)
{
    foo temp(rhs);
    swap(rhs);
    return *this;
}

This assignment function is exception safe and does not need an explicit self-assignment check. What's more the form is the same no matter what the class is.

[GarroteYou]

Thank you both very much for the replies -

Graham, I understand the swap() function method and how it
uses the copy constructor, however, if you have a moment, I'd
apreciate a little more explanation of what I encountered while
upgrading my little project. I've included // coments where
needed.

Here is the new class declaration:

class Array                       //models a normal C++ array
{
public:
	Array(int s)			//one-argument constructor
	{
		size = s;			//argument is size of Array
		ptr = new int[s];		//make space for Array
	}
		
	Array( Array& a )			//overloaded copy constructor
	{
		this->size = a.size;	//copy the vars
		ptr = new int[size];	//create space

				        //copy everything over
		for( int i=0; i<size; i++ )
		{
			*(this->ptr+i) = a[i];
		}		
	}

	~Array()                   //destructor
	{ delete[] ptr; }
	
	int& operator [] ( int j)  //overloaded subscript operator
	{ 
			return *(ptr+j); 
	}
	
		//	What does throw() do here? 
	bool Swap( Array& other ) throw()
	{
		for( int i=0; i<size; i++ )
		{
			*(this->ptr+i) = other[i];
		}
		
		if ( this->size != other.size )
			return false;
		else
			return true;

	}
	
	/* If 'const' is enabled here, 2 error messages are created:
           "class 'Array' : no copy constructor available" referencing 
	   the following line: Array temp( rhs ); 
   
           The second error is: "cannot convert parameter 1 from 'const 
	   class Array' to 'class Array &'"
   
       	   If const is removed, the program compiles properly.
   
    	*/
	Array& operator = (  /* const */ Array& rhs )  
	{
		Array temp( rhs );	// why do we need this?
					// can't we just send rhs to swap? 	
		if( !Swap( temp ) )	// it does work in this example
		{
			cout	<< "Warning: array index sizes do not match:\n\n" 
				<< "this->size == (" << size << ") != " << "temp.size (" 
				<< temp.size << ")";
		}
		return *this;
	}

private:
	int* ptr;	//pointer to Array contents
	int size;	//size of Array
};

[galathaea]

	//	What does throw() do here? 
	bool Swap( Array& other ) throw()

The throw() specifies that the function cannot throw any exceptions. Normally, this is used in swap routines because the temporary swap variable is usually a POD (with associated non-throwing ctor) created on the stack and there is no exception possible on stack creation except for ctor specifics (since the stack running out of room would make the standard exception mechanism mostly non functional). This however does not seem to fit this swap method (even though the temp is on the stack when passed to swap, the swap method by itself does not ensure correct array size -- it relies on the logic of operator = to do this). I think a minor point, but it is a good idea to get in the habit of making the logic guarantees local to the functions that require them, because technically you could reuse the function later incorrectly and violate the exception guarantee.

	/* If 'const' is enabled here, 2 error messages are created:
           "class 'Array' : no copy constructor available" referencing 
	   the following line: Array temp( rhs ); 
   
           The second error is: "cannot convert parameter 1 from 'const 
	   class Array' to 'class Array &'"
   
       	   If const is removed, the program compiles properly.
   
    	*/
	Array& operator = (  /* const */ Array& rhs )  
	{
		Array temp( rhs );	// why do we need this?
					// can't we just send rhs to swap? 	
		if( !Swap( temp ) )	// it does work in this example
		{
			cout	<< "Warning: array index sizes do not match:\n\n" 
				<< "this->size == (" << size << ") != " << "temp.size (" 
				<< temp.size << ")";
		}
		return *this;
	}

Notice the temp copy construction uses rhs. If you make rhs const, you need a copy ctor that takes a const or you will violate the const sompiler agreement. Its usually a good idea to make your default copy ctor use const anyway, since copying should not change the object instance copied. I, too, have some worries about the general logic of this routine and the purpose of the temp. It doesn't seem to be following the normal pattern of temp swapping, and I don't quite see its use here being beneficial. But I may be missing something...

I kind of like the fact that your use of this-> documents which object is being used in multiple object situations, so I just want to say that I appreciate that type of coding style. Since this-> doesn't do anything more than directly accessing the member, its rare to see it used (except in member function pointer uses), but I think its a good self-documenting coding style!

[Graham]

In this case, the swap() function is trivial:

void Array::Swap(Array& other)
{
    // could use std::swap, but written out here for clarity
    int *ptemp = ptr;
    ptr = other.ptr;
    other.ptr = ptemp;

    int sizetemp = size;
    size = other.size;
    other.size = sizetemp;
}

i.e. it is literally swapping the data members. Note that this function cannot throw an exception since we are only doing assignments on fundamental types.

Copy ctor and dtor are needed:

Array::Array() : ptr(0), size(0) {} // Just to be sure...

Array::Array(const Array& other) : size(other.size)
{
    ptr = new int[size];
    std::copy(other.ptr, other.ptr+size, ptr);
}

Array::~Array()
{
    delete [] ptr;
}

To understand how the operator= works, given the above swap and a properly functional copy constructor (and dtor), let's take it line by line:

Array& operator=(const Array& other)
{
    Array temp(other);

Here we're taking a copy of the other array into a temporary variable. If any funny business is going to happen, this is where it will do it. If the new[] in the copy ctor throws, it'll throw constructing this temporary. The important point being that, if it does throw, the actual object remains undamaged - recovery is possible.

    Swap(temp);

If we get to this line, then "temp" was constructed without a problem. In other words, we've got past the possible screw-up. Because Swap() is guaranteed non-throwing, we can safely alter the state of our object without worrying about it.

Note that *this now contains the array that was pointed to inside temp, and temp now points to the memory that *this used to point to.

    return *this;

Return a reference to *this, for chaining - remember, this->ptr now points to memory that contains a copy of the values contained in the memory pointed to by other.ptr. The two "ptr"s are different, but they point to the same information.

When the function exits, "temp" is destroyed, freeing up the memory previously pointed to by *this.

So the sequence is:

1) Construct a temporary that's a copy of the other object. This allocates new memory and copies the data over. This is where any exceptions will be thrown.
2) Swap the data members of *this and "temp". "temp" now points at the old data, *this points at the copy of "other"s data.
3) On return from the function, the destructor cleans up the memory pointed to by "temp" (i.e. the previous contents of *this).

Note that this function is also safe against self-assignment (array = array) - if not totally efficient. Try it and see.