C++ matrix class follow up

Question

This is a matrix class with addition and multiplication. I tried incorporating all the helpful advice I got on my previous attempt C++ matrix class , specifically using std::vector for memory management, overloading operators, adding error messages and adhering to the rule of 5.

#include <iostream>
#include <vector>
template<typename T>
class Matrix {
public:
 // Default constructor
 Matrix();
 // Constructor
 Matrix(int m, int n, std::vector<T> data): rows_{m}, columns_{n} {
 if (m*n != (int) data.size()) {
 throw "Dimensions do not match data!";
 }
 data_ = data;
 }
 
 // Copy constructor (makes a copy of the argument)
 Matrix(const Matrix<T>& other): rows_{other.rows_}, columns_{other.columns_} {
 data_ = other.data_;
 }
 
 // Move constructor (moves the information of the argument to the created object, the 
 // argument is destroyed)
 Matrix(Matrix<T>&& other): rows_{other.rows_}, columns_{other.columns_}, data_{other.data_} {
 other.data_.~vector();
 }
 
 // Copy assignment operator (overloads = when we have an object on the other side of =)
 Matrix<T>& operator=(const Matrix<T>& other) {
 if (this != &other) {
 rows_ = other.rows_;
 columns_ = other.columns_;
 data_ = other.data_;
 }
 
 return *this;
 }
 
 // Move assignment operator (overloads = when we have a reference on the other side =)
 Matrix<T>& operator=(Matrix<T>&& other) {
 if (this != &other) {
 rows_ = other.rows_;
 columns_ = other.columns_;
 data_ = &other.data_;
 other.data_.~vector();
 }
 
 return *this;
 }
 
 // Destructor
 ~Matrix() {
 data_.~vector();
 }
 
 // Returns a reference to the entry at (i, j), so we can change it.
 T& operator()(int i, int j) {
 return data_.at(columns_*i + j); 
 }
 
 Matrix<T> operator+(const Matrix<T>& other) {
 if (columns_ != other.columns_ || rows_ != other.rows_) {
 throw "Addition failure: dimensions do not match";
 }
 
 // (Why doesn't 'Matrix<T> sum = this' work as an application of the copy constructor?)
 Matrix<T> sum(rows_, columns_, data_);
 for (int i = 0; i < rows_*columns_; i++) {
 sum.data_[i] += other.data_[i];
 }
 
 return sum;
 }
 
 // Design question: how can I change operator() to allow other to be constant?
 Matrix<T> operator*(Matrix<T>& other) {
 if (columns_ != other.rows_) {
 throw "Multiplication error: dimensions do not match";
 }
 
 std::vector<T> product;
 
 for (int i = 0; i < rows_; i++) {
 for (int j = 0; j < other.columns_; j++) {
 T sum = 0;
 for (int k = 0; k < columns_; k++) {
 sum += other(k, j) * this->operator()(i, k);
 }
 product.push_back(sum);
 }
 }
 
 return Matrix(rows_, columns_, product);
 }
 
private:
 int rows_;
 int columns_;
 std::vector<T> data_;
 
 template<typename U>
 friend std::ostream& operator<<(std::ostream& outputStream, Matrix<U>& matrix);
};
template<typename U>
std::ostream& operator<<(std::ostream& outputStream, Matrix<U>& matrix) {
 
 for (int i = 0; i < matrix.rows_; i++) {
 outputStream << '{';
 for (int j = 0; j < matrix.columns_ - 1; j++) {
 outputStream << matrix(i, j) << ", ";
 }
 outputStream << matrix(i, matrix.columns_ - 1) << "}\n";
 }
 
 return outputStream;
}
int main() {
 std::vector<int> data{1, 2, 3, 4};
 Matrix<int> mat(2, 2, data);
 
 Matrix<int> sum = mat + mat;
 Matrix<int> product = mat * mat;
 
 std::cout << mat << "+\n" << mat << "=\n" << sum;
 std::cout << "------------------" << std::endl;
 std::cout << mat << "*\n" << mat << "=\n" << product;
 
 mat(0, 0) = 0;
 std::cout << "Changed mat:\n" << mat;
}

• theProgrammer: If you use std::vector the compiler generated copy and move constructor, copy and move assignment is okay, you do not need to redefine it except you manually manage your memory.

Answer 1

• The default constructor Matrix() is declared, but I don't see it defined anywhere. It should initialize rows_ and columns_ to zero.

• Throwing a std::runtime_error or std::invalid_argument exception object would be better than a plain string literal (it's useful to have type information about what went wrong, and easier to use when catching, since they derive from std::exception).

• The move and copy constructors and assignment operators do the same things that we can make the compiler generate with = default;:

  Matrix(const Matrix&) = default;
  Matrix(Matrix&&) = default;
  Matrix& operator=(const Matrix&) = default;
  Matrix& operator=(Matrix&&) = default;
  

  Note that inside the class, we can use Matrix to mean the same as Matrix<T>.

• bug: We must not call the vector destructor explicitly (e.g. data_.~vector()). Member destructors (i.e. including the data_ destructor) are called automatically after the class destructor. We don't need to do it manually. This means we can omit the ~Matrix() function, as the compiler generated version will work fine.

• We need a const version of the element access operator, i.e. T const& operator()(int i, int j) const;. This allows us to access elements in a const Matrix object. (Note: this is why operator* doesn't currently work with a const Matrix&).

• To invoke the copy constructor inside operator+, we need Matrix sum = *this; (Remember that this is a pointer so we have to dereference it).

• We should use std::size_t, not int, for rows_ and columns_ and the corresponding index arguments. This matches the size_type of our vector member. It's also safer: right now Matrix(-2, -2, { 0, 1, 2, 3 }) doesn't trigger the size check exception!

• We could do with some more functionality: it would be useful to have const member functions to return the number of rows(), columns() and the overall size() (rows() * columns()).

  We could also add begin(), end(), rbegin() and rend() functions, so that we could iterate over all the members in a matrix easily (e.g. for (auto v : matrix)). These could call the corresponding std::vector member functions and return std::vector<T>::iterators.

• ALX23z: It is very ill advised to make move-ctor/assignment to be defaulted in such cases. As then moved-from object is in an invalid state where rows/cols don't match with the vector it contains leading to variety of bugs that users will encounter. While it is not strictly required by the language standard, it is a poor design choice.