11
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ What should iterators be good for in mathematical matrix? \$\endgroup\$
    – convert
    Feb 17 at 11:55
  • \$\begingroup\$ I implemented them just to use stl algorithm functionalities, but they can moved all to private), or they can be used only in terms of mData.begin() , mData.end() and not to be inlined in matrix class \$\endgroup\$ Feb 17 at 12:00
  • 1
    \$\begingroup\$ Using mData.begin() and mData.end() looks like the best solution to me. \$\endgroup\$
    – convert
    Feb 17 at 12:23
  • \$\begingroup\$ Okay, will take into consideration, I also noticed some prone errors in my code, will update it) \$\endgroup\$ Feb 17 at 12:53
  • \$\begingroup\$ Do you mean to say -> template<typename T> class Matrix{} \$\endgroup\$ Feb 18 at 14:14

3 Answers 3

6
\$\begingroup\$
  • 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 qualifier

    Matrix operator+(const Matrix &rhs) &&
    

    or make all overload as (friend) free functions.

    You might implement operator + with operator +=.

  • std::vector already has an operator == so

    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());
    }
    

    can be simplified to

    bool operator==(const Matrix &rhs) const {
        return mCols == rhs.mCols && mRows == rhs.mRows && mData == rhs.mData;
    }
    
\$\endgroup\$
4
  • \$\begingroup\$ All your comments are right, will work on them) Thanks) \$\endgroup\$ Feb 17 at 13:50
  • \$\begingroup\$ If I make all overload as friend free functions should not I make friend Matrix operator+(const Matrix& lhs, const Matrix& rhs); friend Matrix operator+(Matrix&& lhs, const Matrix& rhs); friend Matrix operator+(Matrix&& lhs, Matrix&& rhs); friend Matrix operator+(const Matrix& lhs, Matrix&& rhs); Overloads? Is there a good practice of doing this? I've read in Scott Meyer's book somewhere that perfect forwarding may help, is this the case? \$\endgroup\$ Feb 17 at 14:01
  • \$\begingroup\$ If you want to handle each combinations, you need 4 overloads (as methods or as free functions). Forwarding reference won't help here as you need Matrix not T&&. \$\endgroup\$
    – Jarod42
    Feb 17 at 15:53
  • \$\begingroup\$ Since C++20, we can define bool operator==(const Matrix &rhs) const = default; (and we get operator!=() for free). It's unlucky the code is targeting C++17. \$\endgroup\$ Feb 17 at 17:03
2
\$\begingroup\$

Thanks for using vector!

It is conceivable to implement a matrix class with std::unique_ptr<T[]> instead of vector, thereby shaving off 16 bytes (on common platform), and too many make the mistake.

The host of utilities provided by vector (such as resize, and copies) more than make up for the excess 16 bytes, so only in extreme circumstances should it be considered to roll your own.

Data members should be private.

First of all, this class has no virtual functions, therefore it is not meant to be inherited from, and thus protected does not make sense.

Even if it were a base class, however, protected should still be avoided. You cannot maintain invariants concerning protected data-members: they break encapsulation.

R-value setters should move their argument.

Firstly, mData = std::move(data); works even if T itself is not moveable.

Secondly, it is vastly cheaper to execute mData = std::move(data); (just copying 3 pointers) than it is to copy each and every element individually.

As such, setters taking in DataType&& data should just move data into mData directly, rather than performing element-wise moves.

Further considerations

  • Have you considered stronger typing? At the moment it's very easy to accidentally swap rows and columns, using distinct types for each would make this a compile time error.
  • Have you considered indexing? It's painful to have to do matrix.data().at(row * matrix.cols() + col), a provided at (or operator()) would make this so much sweeter.
  • By exposing the data-type so much, you expose whether data is stored in row-major or column-major format. This also prevents nifty optimizations like zero-cost transpose. Is this really necessary?
  • Instead of taking const DataType& consider taking in std::span<const T> where suitable; it avoids building a vector when one doesn't have one handy.
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for a good answer, I'll definitely take these into consideration \$\endgroup\$ Feb 19 at 13:29
0
\$\begingroup\$

Since all binary operators do not modify your Matrix, they should be implemented as non-member functions.

If (and only if) they need access to private members, implement them a friend. Here, this is clearly the case.

Note that you need to declare them as a friend inside the matrix class, but additionaly declare them for the compiler.

class Matrix{
    Matrix& operator*=(Matrix const& rhs); // modidies matrix, so it's a member function


    // friend declaration, i.e. "if a function like this is called, it has private access"

    friend Matrix operator+(Matrix const& lhs, Matrix const& rhs); // not a member function
    // scalar types
    template<typename U>
    friend Matrix operator*(Matrix const& lhs, U const& rhs);
    template<typename U>
    friend Matrix operator*(U const& lhs, Matrix const& rhs);
    // matrix multiplication
    friend Matrix operator*(Matrix const& lhs, Matrix const& rhs);
};

// declaration of the actual function

template<typename U>
Matrix operator*(Matrix const& lhs, U const& rhs){
    auto copy(lhs);
    copy *= rhs; // use this operator here
    return copy;
}

template<typename U>
Matrix operator*(U const& lhs, Matrix const& rhs){
    return rhs * lhs; // call the previous implementation above
}

This has the advantage, that all these operators are found in the same place in your code, and not half of them are member functions (when it's possible) and half of them are not (because double * Matrix can not be defined as a member function).

If you want your Matrix and the type to be templated, you should be aware about decltype

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.