1

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!

Improve this question
6
  • 2
    Your assignment operator signature should look like Matrix& operator=(const Matrix& A). You also need a copy constructor and destructor. See: What is The Rule of Three? Commented Oct 25, 2013 at 1:34
  • @Blastfurnace: Actually, I think the signature is absolutely the correct one! However, the implementation is not! ;-) Commented Oct 25, 2013 at 1:52
  • @DietmarKühl: I could agree with using Matrix& operator=(Matrix A) instead. Commented Oct 25, 2013 at 2:06
  • @Blastfurnace: OK, I didn't notice that the Matrix was also returned by value: that is, indeed, a Bad Idea. Passing the argument by value is quite reasonable as a copy is needed anyway (see my answer). Commented Oct 25, 2013 at 2:08
  • @Dietmar Kühl: How would i change the implementation then? I did already have a destructor and a copy constructor, I just didnt bother to show them here, thinking they wouldnt affect the answer to my question. What's wrong with the implementation though? Thanks again for the help! Commented Oct 25, 2013 at 2:09

1 Answer 1

Reset to default
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.

Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

Thank you! This definitely helped. I hadn't considered using std::swap on an object passed in by value. I was actually using exception handling at first, but since I was declaring (very) small arrays to test it with, I just wanted to make sure the fundamental ops worked first.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.