C++ - What is wrong with my move/copy constructors and move/copy assign operators?

Question

So I have my own matrix class and I have tried to overload the + and += operators for the class like this:

template <class T>
void Matrix<T>::operator+=(Matrix& mat) {
  if (this->m_rows != mat.m_rows || this->m_cols != mat.m_cols) {
    cerr << "Matrix rows/cols don't match" << endl;
  }
  else {
    for (unsigned int i = 0; i < this->m_rows; i++) {
      for (unsigned int j = 0; j < this->m_cols; j++) {
        this->m_vec[i * this->m_cols + j] += mat.m_vec[i * this->m_cols + j];
      }
    }
  }
}

template <class T>
Matrix<T> operator+(Matrix<T> mat1, Matrix<T>& mat2) {
  if (mat1.get_rows() != mat2.get_rows() || mat1.get_cols() != mat2.get_cols()) {
    cerr << "Matrix rows/cols don't match" << endl;
  }
  else {
    mat1 += mat2;
    return mat1;
  }
}

(I am attempting to make the + operator chainable and the += non-chainable. Matrix is represented by a one-dimensional array)

However, when I try to add two matrices, the program refuses to add them (no error message). I am suspecting that there may be something wrong with either my move/copy constructors, move/copy assignment operators or my destructor, but I can't figure out what is wrong.

// Copy constructor
template <class T>
Matrix<T>::Matrix(Matrix& obj) {
  size_t rows = obj.get_rows();
  size_t cols = obj.get_cols();
  this->m_rows = rows;
  this->m_cols = cols;
  this->m_capacity = rows * cols;
  this->m_vec = new T[rows * cols];
  for (int i = 0; i < rows; i++) {
    for (int j = 0; j < cols; j++) {
      this->m_vec[j + i * cols] = obj(i, j);
    }
  }
}

// Move constructor
template <class T>
Matrix<T>::Matrix(Matrix&& obj)
  : m_rows(0)
  , m_cols(0)
  , m_capacity(0)
  , m_vec(nullptr)
{
  m_rows = obj.get_rows();
  m_cols = obj.get_cols();
  m_capacity = obj.get_capacity();
  m_vec = obj.m_vec;

  obj.m_rows = 0;
  obj.m_cols = 0;
  obj.m_capacity = 0;
  obj.m_vec = nullptr;
}

// Destructor
template <class T>
Matrix<T>::~Matrix() {
  delete[] m_vec;
}

// Copy assignment operator
template <class T>
Matrix<T>& Matrix<T>::operator=(const Matrix& obj)
{
  m_rows = obj.get_rows();
  m_cols = obj.get_cols();
  m_capacity = obj.get_capacity();
  m_vec = obj.m_vec;
  return *this;

}

// Move assignment operator
template <class T>
Matrix<T>& Matrix<T>::operator=(Matrix&& obj)
{
  if (this != &obj)
  {
    delete[] m_vec;
    m_rows = obj.get_rows();
    m_cols = obj.get_cols();
    m_capacity = obj.get_capacity();
    m_vec = obj.m_vec;

    obj.m_rows = 0;
    obj.m_cols = 0;
    obj.m_capacity = 0;
    obj.m_vec = nullptr;
  }
  return *this;
}

// Matrix class properties
class Matrix{
private:
  size_t m_rows;
  size_t m_cols;
  size_t m_capacity;
  T* m_vec;
}

I am quite new to C++ and this is my first time attempting to create copy/move constructors/assignment operators, so there might be something obvious that I am missing. Maybe I am even overcomplicating this. Is there an easier/better way to write move/copy constructors and assignment operators? Are you able to spot anything strange here? It seems fine to me...

Thanks!

EDIT: I have now tried to change the copy assignment operator as suggested by @eerorika, but my + and += operators still don't work (same as before). Just to be sure, I then also tried to edit my move assignment operator and move constructor in the same way as I edited my copy assignment operator and still... nothing. Here are the edited versions

// Move constructor
template <class T>
Matrix<T>::Matrix(Matrix&& obj)
  : m_rows(0)
  , m_cols(0)
  , m_capacity(0)
  , m_vec(nullptr)
{
  this->m_rows = obj.get_rows();
  this->m_cols = obj.get_cols();
  this->m_capacity = obj.get_capacity();
  //m_vec = obj.m_vec;
  this->m_vec = new T[obj.get_rows() * obj.get_cols()];
  for (int i = 0; i < obj.get_rows(); i++) {
    for (int j = 0; j < obj.get_cols(); j++) {
      this->m_vec[j + i * obj.get_cols()] = obj.m_vec[i * this->m_cols + j];
    }
  }

  obj.m_rows = 0;
  obj.m_cols = 0;
  obj.m_capacity = 0;
  obj.m_vec = nullptr;
}

// Move assigment operator
template <class T>
Matrix<T>& Matrix<T>::operator=(Matrix&& obj)
{
  // If the address of the object being passed in is the same as "this", do nothing.
  if (this != &obj)
  {
    delete[] m_vec;
    this->m_rows = obj.get_rows();
    this->m_cols = obj.get_cols();
    this->m_capacity = obj.get_capacity();
    //m_vec = obj.m_vec;
    this->m_vec = new T[obj.get_rows() * obj.get_cols()];
    for (int i = 0; i < obj.get_rows(); i++) {
      for (int j = 0; j < obj.get_cols(); j++) {
        this->m_vec[j + i * obj.get_cols()] = obj.m_vec[i * this->m_cols + j];
      }
    }

    obj.m_rows = 0;
    obj.m_cols = 0;
    obj.m_capacity = 0;
    obj.m_vec = nullptr;
  }
  return *this;
}

// Copy assignment operator
template <class T>
Matrix<T>& Matrix<T>::operator=(Matrix& obj)
{
  this->m_rows = obj.get_rows();
  this->m_cols = obj.get_cols();
  this->m_capacity = obj.get_capacity();
  //m_vec = obj.m_vec;
  this->m_vec = new T[obj.get_rows() * obj.get_cols()];
  for (int i = 0; i < obj.get_rows(); i++) {
    for (int j = 0; j < obj.get_cols(); j++) {
      this->m_vec[j + i * obj.get_cols()] = obj.m_vec[i * this->m_cols + j];
    }
  }
  return *this;
}

Any idea of what might be wrong now? Are you sure that there is nothing wrong with the code in my + and += operators? Help.

EDIT 2:

Due to a request by @numzero I will add the code for my operator()

template <class T>
T& Matrix<T>::operator()(unsigned int row, unsigned int col) {
  if (row > this->m_rows || col > this->m_cols) {
    cerr << "index out of range" << endl;;
  }
  return this->m_vec[(row - 1) * m_cols + (col - 1)];
}

Answer 1

What is wrong with my move/copy constructors and move/**** assign operators?

Nothing serious as far as I can tell. One minor issue is that you unnecessarily assign the members that you could have initialised with those values directly in the member initialiser list.

What is wrong with my copy assign operators?

Your destructor deletes the member pointer. Because of this, you must maintain a class invariant that every instance of the class has unique ownership of the pointer i.e. no other instance has the same pointer value. Otherwise the destructor of the respective objects would attempt to delete the same pointer which results in undefined behaviour.

Your copy assign operator copies the pointer which violates that class invariant. See your copy constructor for an example of what you're supposed to do instead.

---

Bonus advice: Don't ever use owning bare pointers like you do here. Use smart pointers or RAII containers such as std::vector instead.

Answer 2

Your copy assignment operator copies the m_vec pointer, instead of copying its contents (like the copy constructor does).