How to overload operator= in a templated class with a dynamically sized array

Question

I'm new to making my own template classes in C++, and after several hours of searching online for answers and toying with the function & its parameters, I've given up. I'm having run-time trouble with the following class' "=" operator:

In matrix.h:

template <class datatype> class Matrix{
  datatype** element;
  unsigned int m,n;

  public:

  Matrix(unsigned int M, unsigned int N,datatype x){
    m=M;    // # of rows
    n=N;    // # of cols
    element=new datatype*[m];
    for(int i=0;i<m;i++) element[i]=new datatype[n];
    for(int i=0;i<m;i++)
      for(int j=0;j<n;j++)
        element[i][j]=x;
  }

  void print(){
    for(int i=0;i<m;i++){
      for(int j=0;j<n;j++) cout<<element[i][j]<<" ";
      cout<<"\n";
    }
  }

  Matrix operator=(Matrix A){
    for(int i=0;i<m;i++) delete[] element[i];
    delete[] element;
    m=A.m;
    n=A.n;
    element=new datatype*[m];
    for(int i=0;i<m;i++) element[i]=new datatype[n];
    for(int i=0;i<m;i++)
      for(int j=0;j<n;j++)
        element[i][j]=A.element[i][j];
    return *this;
  }
};

When I go to test this, compilation & linking run smoothly w/o error, and I get a perfectly valid print. But when trying to assign one matrix to the value of another, the program crashes w/ message "matrix_test has stopped working". Here's my testing code, in matrix_test.cpp:

Matrix<int> M(5u,3u,0);
Matrix<int> P(2u,7u,3);

int main(){
    M.print();
    cout<<"\n";
    P.print();
    cout<<"\n";
    P=M;
    P.print();        
}

Thanks in advance for the help!

Answer 1

First off, the implementation of your copy-assignment is flawed in a rather fundamental way: When you delete[] the representation and then allocate a new copy, the allocation may throw in which case your original matrix is delete[]d and can't be recovered. Thus, the assignment isn't exception safe.

The best implementation of the copy-assignment operator is to leverage the copy-construction and the swap() member. Of course, both of these members are missing from your class but let's get to this later:

Matrix& Matrix::operator= (Matrix other) {
    other.swap(*this);
    return *this;
}

When passing an argument by value, it is actually getting copied. To copy the object, you'll need a copy constructor. In general, if you need a copy-assignment you typically also need a copy constructor and a destructor (there are cases when you only need a copy-assignment to make the copy-assignment strongly exception-safe but that's a different discussion).

The purpose of the copy constructor is to copy another object, e.g., when the object is passed by value:

Matrix::Matrix(Matrix const& other)
    : element(new datatype*[other.m])
    , m(other.m)
    , n(other.n)
{
    int count(0);
    try {
        for (; count != m; ++count) {
            this->element[count] = new datatype[m];
            std::copy(other.element[count], other.element[count] + m,
                      this->element[count]);
        }
    }
    catch (...) {
        while (count--) {
            delete[] this->element[count];
        }
        delete[] this->element;
        throw;
    }
}

I'm not sure if the recovery from an exception is actually correct: I can't deal with the complexity of coping with all these pointers! In my code I would make sure that all resources immediately constructor an object dedicated to releasing them automatically but that would require to change the type of the objects. Given the definition of the type, a destructor is also needed:

Matrix::~Matrix() {
    for (int count(this->m); count--; ) {
        delete[] this->element[count];
    }
    delete[] this->element;
}

Finally, for larger objects a swap() member is often handy. The purpose of swap() is to just exchange the content of two objects. The way to implement it is to do a memberwise std::swap():

void Matrix::swap(Matrix& other) {
    using std::swap;
    swap(this->element, other.element);
    swap(this->n, other.n);
    swap(this->m, other.m);
}

Given that all members of this class are built-in types (although they probably shouldn't be), the using-dance isn't really needed. However, if there are specialized overloads of swap() in other namespaces than std::swap() for user-defined types, the above approach makes sure these are found by argument dependent look-up.