I am trying to implement an optimal and fast running matrix in C++. I need some review of the code and ideas on how to improve the code quality if it shall be.
class Matrix { // Some static assertions and useful types static_assert(std::is_arithmetic_v<T>, "Matrix template parameter type must be arithmetic"); using DataType = std::vector<T>; // Default constructors public: Matrix() : mCols(0), mRows(0), mData(0) { }; Matrix(const Matrix &other) = default; Matrix(Matrix &&other) noexcept = default; Matrix &operator=(const Matrix &other) = default; Matrix &operator=(Matrix &&other) noexcept = default; // Parameterized constructors public: Matrix(std::size_t cols, std::size_t rows) : mCols(cols), mRows(rows) { mData.resize(cols * rows); } Matrix(std::size_t cols, std::size_t rows, const DataType &data) { if (rows * cols != data.size()) { throw std::invalid_argument("Invalid matrix mData"); } mCols = cols; mRows = rows; mData = data; } Matrix(std::size_t cols, std::size_t rows, DataType &&data) { if (rows * cols != data.size()) { throw std::invalid_argument("Invalid matrix mData"); } mCols = cols; mRows = rows; mData = std::move(data); } // Setters public: void set(std::size_t rows, std::size_t cols, const DataType &data) { if (rows * cols != data.size()) { throw std::invalid_argument("Invalid vector data"); } mRows = rows; mCols = cols; mData.resize(rows * cols); std::copy(data.begin(), data.end(), mData.begin()); } void set(std::size_t rows, std::size_t cols, DataType &&data) { if (rows * cols != data.size()) { throw std::invalid_argument("Invalid vector data"); } mRows = rows; mCols = cols; mData.resize(rows * cols); std::move(data.begin(), data.end(), mData.begin()); } void set(std::size_t rows, std::size_t cols) { mData.resize(rows * cols); } void set(const DataType &data) { if (mRows * mCols != data.size()) { throw std::invalid_argument("Invalid vector data"); } std::copy(data.begin(), data.end(), mData.begin()); } void set(DataType &&data) { if (mRows * mCols != data.size()) { throw std::invalid_argument("Invalid vector data"); } std::move(data.begin(), data.end(), mData.begin()); } // Getters public: auto cols() const { return mCols; } auto rows() const { return mRows; } const DataType &data() const { return mData; } // Operator overloads public: Matrix operator+(const Matrix &rhs) const { if (mCols != rhs.mCols || mRows != rhs.mRows) { throw std::invalid_argument("Invalid vector mData, you may add matrices with same dimensions only"); } Matrix result(mCols, mRows); std::transform(mData.begin(), mData.end(), rhs.mData.begin(), result.mData.begin(), std::plus<>()); return result; } Matrix operator+(Matrix &&rhs) const { if (mCols != rhs.mCols || mRows != rhs.mRows) { throw std::invalid_argument("Invalid vector mData, you may add matrices with same dimensions only"); } Matrix result(mCols, mRows); std::transform(mData.begin(), mData.end(), std::make_move_iterator(rhs.mData.begin()), result.mData.begin(), std::plus<>()); return result; } Matrix operator-(const Matrix &rhs) const { if (mCols != rhs.mCols || mRows != rhs.mRows) { throw std::invalid_argument("Invalid vector mData, you may sub matrices with same dimensions only"); } Matrix result(mCols, mRows); std::transform(mData.begin(), mData.end(), rhs.mData.begin(), result.mData.begin(), std::minus<>()); return result; } Matrix operator-(Matrix &&rhs) const { if (mCols != rhs.mCols || mRows != rhs.mRows) { throw std::invalid_argument("Invalid vector mData, you may sub matrices with same dimensions only"); } Matrix result(mCols, mRows); std::transform(mData.begin(), mData.end(), std::make_move_iterator(rhs.mData.begin()), result.begin(), std::minus<>()); return result; } Matrix &operator+=(const Matrix &rhs) { if (mCols != rhs.mCols || mRows != rhs.mRows) { throw std::invalid_argument("Invalid vector mData, you may add matrices with same dimensions only"); } std::transform(mData.begin(), mData.end(), rhs.mData.begin(), mData.begin(), std::plus<>()); return *this; } Matrix &operator+=(Matrix &&rhs) { if (mCols != rhs.mCols || mRows != rhs.mRows) { throw std::invalid_argument("Invalid vector mData, you may add matrices with same dimensions only"); } std::transform(mData.begin(), mData.end(), std::make_move_iterator(rhs.mData.begin()), mData.begin(), std::plus<>()); return *this; } Matrix &operator-=(const Matrix &rhs) { if (mCols != rhs.mCols || mRows != rhs.mRows) { throw std::invalid_argument("Invalid vector mData, you may add matrices with same dimensions only"); } std::transform(mData.begin(), mData.end(), rhs.mData.begin(), mData.begin(), std::minus<>()); return *this; } Matrix &operator-=(Matrix &&rhs) { if (mCols != rhs.mCols || mRows != rhs.mRows) { throw std::invalid_argument("Invalid vector mData, you may add matrices with same dimensions only"); } std::transform(mData.begin(), mData.end(), std::make_move_iterator(rhs.mData.begin()), mData.begin(), std::minus<>()); return *this; } bool operator==(const Matrix &rhs) const { return mCols == rhs.mCols && mRows == rhs.mRows && std::equal(mData.begin(), mData.end(), rhs.mData.begin(), rhs.mData.end()); } bool operator!=(const Matrix &rhs) const { return !(*this == rhs); } friend std::ostream &operator<<(std::ostream &os, const Matrix &matrix) { for (int i = 0; i < matrix.mCols; ++i) { for (int j = 0; j < matrix.mRows; ++j) { os << matrix.mData[i * matrix.mCols + j] << " "; } os << "\n"; } return os; } protected: std::size_t mCols; std::size_t mRows; std::vector<T> mData; };
I’ll add determinant and matrix multiplication operators too, but that will come after reviewing this part.
Thanks in advance.
Answer
-
for
Matrix() : mCols(0), mRows(0), mData(0) { };
semicolon should be removed as useless.
-
for
Matrix(std::size_t cols, std::size_t rows) : mCols(cols), mRows(rows) { mData.resize(cols * rows); }
You can use member list initialization too for
vector
:Matrix(std::size_t cols, std::size_t rows) : mCols(cols), mRows(rows), mData(cols * rows) {}
-
for
void set(std::size_t rows, std::size_t cols) { mData.resize(rows * cols); }
it is wrong, as changing row/columns change internal layout, so for example element of 2nd row, 2nd column won’t be there anymore.
resize
seems to be a better name if you keep that function. -
for arithmetic operators
Matrix operator+(const Matrix &rhs) const
, you forget when lhs is a rvalue, so either add overload with this qualifierMatrix operator+(const Matrix &rhs) &&
or make all overload as (
friend
) free functions.You might implement
operator +
withoperator +=
. -
std::vector
already has anoperator ==
sobool operator==(const Matrix &rhs) const { return mCols == rhs.mCols && mRows == rhs.mRows && std::equal(mData.begin(), mData.end(), rhs.mData.begin(), rhs.mData.end()); }
can be simplified to
bool operator==(const Matrix &rhs) const { return mCols == rhs.mCols && mRows == rhs.mRows && mData == rhs.mData; }
Attribution
Source : Link , Question Author : Hrant Nurijanyan , Answer Author : Jarod42