copy constructor...what is wrong?

Question

So my problem is this...when I am trying to allocate memory to a pointer, it fails.

This is my MatrixClass definition..

class MatrixClass
{
public:
    MatrixClass(int m, int n);
    MatrixClass(void);
    virtual ~MatrixClass(void);
    double getRead(int num1, int num2) const;
    double& getReadWrite(int num3, int num4);
    void set(int i,int j,double value);//set value at i,j to value
    MatrixClass(const MatrixClass &rhs);
    void assign(int M,int N);
    MatrixClass sum1(const MatrixClass& rhs) const;
    MatrixClass operator+(const MatrixClass& rhs);//overloading the + operator 
    MatrixClass operator-();
private:
    double* dataptr;
    int M;
    int N;
};

I am trying to do this..

MatrixClass BB;
BB = A + B;

So here is my overloaded + function..

MatrixClass MatrixClass::operator +(const MatrixClass &rhs)
{
    MatrixClass temp;
    //temp.M = this->M + rhs.M;
    //temp.N = this->N + rhs.N;
    for(int i = 0;i < M;i++)
    {
        for(int j = 0; j<N;j++)
        {
            temp.dataptr[i * N + j] = this->getReadWrite(i,j) + rhs.dataptr[i*N+j];
        }
    }   
    return temp;
}//end operator +

Once temp returns...it calls the copy constructor...passing temp as 'rhs' and 'this' would refer to 'BB'? (Am I right in thinking this?)

MatrixClass::MatrixClass(const MatrixClass &rhs)//copy constructor
{
    this->M = rhs.M;
    this->N = rhs.N;
    dataptr = 0;
    if(rhs.dataptr != 0)
    {
        dataptr = new double[M * N];//allocate memory for the new object being assigned to...
        // the line here where I try to allocate memory gives me an error.....Am I right in
        //thinking that this would be assigning memory to dataptr of 'BB'?? Values are assigned to //'M' and 'N' fine....
        int num = sizeof(double);
        memcpy(this->dataptr,rhs.dataptr,M*N*sizeof(double));   
    }
    else
    {
        this->dataptr = 0;
    }
}//end copy constructor

Also the error that i get is this... 'Unhandled exception at 0x75a0b727 in assignment.exe: Microsoft C++ exception: std::bad_alloc at memory location 0x002af924..'

So basically the qestion I am asking is..why the hell is is the line where I am trying to allocate memory to 'dataptr' in the copy constructor giving me problems..it only does this when calling the copy constructor from the return value 'temp'..

Thanks!

Answer 1

Short answer: you have not implemented Rule of Three properly. In C++11, Rule of Five.

Long answer: you have not implemented operator=. The compiler generated one is not enough for your case.

Apart from that my guess is that your default constructor doesn't allocate memory for dataptr. If so, then you're using dataptr in operator+ without even allocating memory for it. That is the reason why your program crashes. Even if the default constructor allocates memory, the problem is still there as the value of rhs.M and rhs.N may be different from what you assume in the default constructor (which is, as you said, 4 and 5 respectively).

So allocate memory for dataptr in operator+. Also, before allocation, you've to deallocate the memory which it may point to, and you also need to set M and N to the values of rhs.M and rhs.N respectively.

Answer 2

No, James, you're wrong, when operator+ returns the copy constructor will be called to covert local variable temp into a temporary anonymous object in the scope of the assignment operator. In the copy constructor, this will refer to this temporary variable, not BB.

Because the LHS already exists, it will not be "constructed" again when operator+ returns, it will be "assigned", so you need to overload operator=. (See here for much more detail.) The default operator= just makes a bitwise copy, so if you didn't overload operator=, the dataptr would be freed (we hope) in the destructor of the anonymous temporary object when it is destroyed as it goes out of scope (which is immediately after it is copied) and BB would be pointing to freed memory. You would also leak any memory already allocated in BB as BB would not be destroyed before being assigned. BB's dataptr would leak while the temporary object's dataptr would be freed twice.

So, although it kind of sucks that you end up allocating and copying memory three times, in order to make + and = work in all situations, you need to do the following:

• In operator+ change MatrixClass temp; to MatrixClass temp(rhs.M, rhs.N); so that temp.dataptr is properly allocated and deallocated.
• As you are doing, in the copy constructor allocate new memory for dataptr and copy it over.
• Overload operator= so that it first checks for self-assignment (this == &rhs) and if it is not self-assigning, frees the existing dataptr and then allocates a new one and copies the rhs.