6
\$\begingroup\$

I am a beginner C++ programmer and I have been working on a library to use for working with matrices. The library main features should be

  1. Working with matrices (addition, subtraction, arbitrary operations)
  2. Lazy Evaluation and Reference Counting
  3. Modifiable underlying container

Of course, all of this trying to maintain an acceptable performance.

Any feedback is appreciated, however I am in particular interested in knowing about efficiency issues, violated design principles and any red flags I may have missed.

The structure of the code is the following:

  • Matrix.hpp and MatrixFactory.hpp are responsible for creating and operating on a Matrix<T, container> where T is the type of the elements in the matrix, and container the underlying container
  • MatrixLazyEvaluator.hpp is responsible for almost all of the lazy evaluation
  • RCObject.hpp, RCObject.cpp, RCPtr.hpp are responsible for Reference Counting, and they are shamelessly stolen from 'More Effective C++'
  • MatrixStorage.hpp, DynamicMatrixStorage.hpp, StaticMatrixStorage.hpp are the underlying containers that we use for Matrix. Any user wishing to provide their own implementation will need to implement MatrixStorage.hpp
  • Array_Wrapper.hpp is simply a wrapper for T[] (std::array was not suitable because of the compile time size)
  • main.cpp contains some tests to show intended behaviour

GitHub Repo (MML)

Matrix.hpp

#pragma once

#include <functional>
#include <algorithm>
#include <ostream>

#include "MatrixStorage.hpp"
#include "RCObject.hpp"
#include "RCPtr.hpp"
#include "MatrixLazyEvaluator.hpp"

namespace MML
{

    template <class T, class container = Array_Wrapper<T>>
    class Matrix
    {
    public:
        explicit Matrix(MatrixStorage<T, container>* storage) noexcept :
            _matrix_value{ new MatrixValue(storage) }, _lazy(this) {}


        class ValueProxy
        {
        public:
            ValueProxy(Matrix<T, container>& matrix, const size_t& row, const size_t& col);

            ValueProxy& operator=(const ValueProxy& rhs); // WRITE
            ValueProxy& operator=(const T& type);         // WRITE

                     operator T() const; // READ
            const T* operator& () const; // READ
                  T* operator& ();      // WRITE

        private:
            Matrix<T, container>& _thematrix;
            const size_t& _row;
            const size_t& _col;
        };

              ValueProxy at(const size_t& i, const size_t& j);
        const ValueProxy at(const size_t& i, const size_t& j) const;
                   void set(const size_t& i, const size_t& j, const T& val);

        size_t number_of_rows()    const;
        size_t number_of_columns() const;

        void unary_operation (std::function<T(const T&)>func);
        void binary_operation(std::function<T(const T&, const T&)>, const Matrix& other);

        Matrix& operator+=(const Matrix& other);
        Matrix& operator-=(const Matrix& other);

        template <class X>
        Matrix& operator*=(const X& scalar);

        template <class X>
        Matrix& operator/=(const X& scalar);

        bool operator==(const Matrix& other) const;

        friend std::ostream& operator<<(std::ostream& os, const Matrix<T, container>& other)
        {
            other._lazy.apply_all_changes();
            const size_t rows = other.number_of_rows();
            const size_t cols = other.number_of_columns();

            for (size_t i = 0; i < rows; ++i)
            {
                for (size_t j = 0; j < cols; ++j)
                {
                    os << other.at(i, j);
                    os << ' ';
                }
                os << '\n';
            }

            return os;
        }

    private:
        struct MatrixValue : RCObject
        {
            MatrixValue(const MatrixStorage<T, container>* storage) :
                _storage(storage->clone()) {}
            MatrixValue(const MatrixValue& rhs) :
                _storage(rhs._storage->clone()) {}

            ~MatrixValue() { delete _storage; }

            MatrixStorage<T, container>* _storage;
        };

        RCPtr<MatrixValue> _matrix_value;

        friend class MatrixLazyEvaluator<T, container>;
        mutable MatrixLazyEvaluator<T, container> _lazy;

        void write_unary (const std::function<T(const T&)>& func);
        void write_binary(const std::function<T(const T&, const T&)>& func, const Matrix& other);
        void copy_on_write();
    };

    template <class T, class container>
    typename Matrix<T, container>::ValueProxy Matrix<T, container>::at(const size_t& i, const size_t& j)
    {
        return ValueProxy(*this, i, j);
    }

    template <class T, class container>
    const typename Matrix<T, container>::ValueProxy Matrix<T, container>::at(const size_t& i, const size_t& j) const
    {
        return ValueProxy(const_cast<Matrix&>(*this), i, j);
    }

    template <class T, class container>
    void Matrix<T, container>::set(const size_t& i, const size_t& j, const T& val)
    {
        at(i, j) = val;
    }

    template <class T, class container>
    size_t Matrix<T, container>::number_of_rows() const
    {
        return _matrix_value->_storage->get_number_of_rows();
    }

    template <class T, class container>
    size_t Matrix<T, container>::number_of_columns() const
    {
        return _matrix_value->_storage->get_number_of_columns();
    }

    template <class T, class container>
    void Matrix<T, container>::unary_operation(std::function<T(const T&)> func)
    {
        _lazy.push_unary(func);
    }

    template <class T, class container>
    void Matrix<T, container>::binary_operation(std::function<T(const T&, const T&)> func, const Matrix& other)
    {
        other._lazy.apply_all_changes();
        _lazy.push_binary(func, other);
    }

    template <class T, class container>
    Matrix<T, container>& Matrix<T, container>::operator+=(const Matrix& other)
    {
        binary_operation([](const T& a, const T& b) { return a + b; }, other);
        return *this;
    }

    template <class T, class container>
    Matrix<T, container> operator+(const Matrix<T, container>& lhs, const Matrix<T, container>& rhs)
    {
        auto x{lhs};
        return x += rhs;
    }

    template <class T, class container>
    Matrix<T, container>& Matrix<T, container>::operator-=(const Matrix& other)
    {
        binary_operation([](const T& a, const T& b) { return a - b; }, other);
        return *this;
    }

    template <class T, class container>
    Matrix<T, container> operator-(const Matrix<T, container>& lhs, const Matrix<T, container>& rhs)
    {
        auto x{lhs};
        return x -= rhs;
    }

    template <class T, class container>
    template <class X>
    Matrix<T, container>& Matrix<T, container>::operator*=(const X& scalar)
    {
        unary_operation([&scalar](const T& val) { return val * scalar; });
        return *this;
    }

    template <class T, class container, class X>
    inline Matrix<T, container> operator*(const Matrix<T, container>& lhs, const X& rhs)
    {
        auto temp{ lhs };
        return temp *= rhs;
    }

    template <class T, class container, class X>
    Matrix<T, container> operator*(const X& lhs, const Matrix<T, container>& rhs) { return rhs * lhs; }

    template <class T, class container>
    template <class X>
    Matrix<T, container>& Matrix<T, container>::operator/=(const X& scalar)
    {
        unary_operation([&scalar](const T& val) { return val / scalar; });
        return *this;
    }

    template <class T, class container, class X>
    Matrix<T, container> operator/(const Matrix<T, container>& lhs, const X& rhs)
    {
        auto temp{ lhs };
        return temp /= rhs;
    }

    template <class T, class container>
    bool Matrix<T, container>::operator==(const Matrix& other) const
    {
        _lazy.apply_all_changes();
        other._lazy.apply_all_changes();
        return std::equal(_matrix_value->_storage->begin(), _matrix_value->_storage->end(), other._matrix_value->_storage->begin(), other._matrix_value->_storage->end());
    }

    template <class T, class container>
    void Matrix<T, container>::write_unary(const std::function<T(const T&)>& func)
    {
        copy_on_write();
        _matrix_value->markUnshareable();
        std::for_each(_matrix_value->_storage->begin(), _matrix_value->_storage->end(), [&func](T& val) { val = func(val); });
    }

    template <class T, class container>
    void Matrix<T, container>::write_binary(const std::function<T(const T&, const T&)>& func, const Matrix& other)
    {
        copy_on_write();
        _matrix_value->markUnshareable();
        size_t i = 0;
        for (auto& ref : *_matrix_value->_storage)
        {
            ref = func(ref, (*other._matrix_value->_storage)[i]);
            ++i;
        }
    }

    template <class T, class container>
    void Matrix<T, container>::copy_on_write()
    {
        if (_matrix_value->isShared())
        {
            _matrix_value = new MatrixValue(_matrix_value->_storage);
        }
    }


    // ValueProxy functions -----------------------------------------------------------------------------------------------------------------

    template <class T, class container>
    Matrix<T, container>::ValueProxy::ValueProxy(Matrix<T, container>& matrix, const size_t& row, const size_t& col) :
        _thematrix(matrix), _row(row), _col(col)
    {
    }

    template <class T, class container>
    typename Matrix<T, container>::ValueProxy& Matrix<T, container>::ValueProxy::operator=(const ValueProxy& rhs)
    {
        _thematrix._lazy.apply_all_changes();
        _thematrix._matrix_value->_storage->at(_row, _col) = rhs._thematrix._matrix_value->_storage->at(_row, _col);
        return *this;
    }

    template <class T, class container>
    typename Matrix<T, container>::ValueProxy& Matrix<T, container>::ValueProxy::operator=(const T& type)
    {
        _thematrix._lazy.apply_all_changes();
        _thematrix._matrix_value->_storage->at(_row, _col) = type;
        return *this;
    }

    template <class T, class container>
    Matrix<T, container>::ValueProxy::operator T() const
    {
        return _thematrix._matrix_value->_storage->at(_row, _col);
    }

    template <class T, class container>
    const T* Matrix<T, container>::ValueProxy::operator&() const
    {
        return &(_thematrix._matrix_value->_storage->at(_row, _col));
    }

    template <class T, class container>
    T* Matrix<T, container>::ValueProxy::operator&()
    {
        _thematrix._lazy.apply_all_changes();
        _thematrix._matrix_value->markUnshareable();
        return &(_thematrix._matrix_value->_storage->at(_row, _col));
    }
}

MatrixFactory.hpp

#pragma once

#include "Matrix.hpp"
#include "DynamicMatrixStorage.hpp"
#include "StaticMatrixStorage.hpp"


namespace MML
{
    template <class T>
    class MatrixFactory
    {
    public:
         MatrixFactory() = delete;
        ~MatrixFactory() = delete;


    static Matrix<T> create_matrix(const size_t& rows, const size_t& cols, T* collection = new T[rows*cols])
    {
        Array_Wrapper<T> arr{ collection, rows*cols };
        auto storage = new DynamicMatrixStorage<T>{ rows, cols, arr };
        return Matrix<T>(storage);
    }

    template<size_t rows, size_t cols>
    static Matrix<T> create_matrix(T* collection = new T[rows*cols])
    {
        Array_Wrapper<T> arr{ collection, rows*cols };
        auto storage = new StaticMatrixStorage<T, rows, cols>{ arr };
        return Matrix<T>(storage);
    }

    static Matrix<T> initialise_matrix(const size_t& rows, const size_t& cols, std::function<T(const size_t&, const size_t&)>);

private:
    static void init_storage(MatrixStorage<T>* storage, std::function<T(const size_t&, const size_t&)>);

};

template <class T>
void MatrixFactory<T>::init_storage(MatrixStorage<T>* storage, std::function<T(const size_t&, const size_t&)> init)
{
    for (size_t i = 0; i < storage->get_number_of_rows(); i++)
    {
        for (size_t j = 0; j < storage->get_number_of_columns(); j++)
        {
            storage->set(i, j, init(i, j));
        }
    }
}

template <class T>
Matrix<T> MatrixFactory<T>::initialise_matrix(const size_t& rows, const size_t& cols, std::function<T(const size_t&, const size_t&)> init)
{
    Array_Wrapper<T> arr{ rows*cols };
    auto storage = new DynamicMatrixStorage<T>{ rows, cols, arr };

    init_storage(storage, init);

    return Matrix<T>(storage);
}
}

MatrixLazyEvaluator.hpp

#pragma once

#include <queue>
#include <functional>
#include <memory>

namespace MML

{

template <class T, class container>
class Matrix;

template <class T, class container>
class MatrixLazyEvaluator
{
public:
    explicit MatrixLazyEvaluator(Matrix<T, container>* owner) :
        _owner(owner) {}

    MatrixLazyEvaluator(const MatrixLazyEvaluator& other) noexcept
    {
        _function_queue = other._function_queue;
        _unary_queue    = other._unary_queue;
        _binary_queue   = other._binary_queue;
        _owner          = other._owner;
    }

    MatrixLazyEvaluator& operator=(MatrixLazyEvaluator other) noexcept
    {
        swap(*this, other);
        return *this;
    }

    MatrixLazyEvaluator(MatrixLazyEvaluator&& other) noexcept
    {
        swap(*this, other);
    }

    MatrixLazyEvaluator& operator=(MatrixLazyEvaluator&& other) noexcept
    {
        apply_all_changes();
        swap(*this, other);
        return *this;
    }

    friend void swap(MatrixLazyEvaluator& lhs, MatrixLazyEvaluator& rhs) noexcept
    {
        using std::swap;
        swap(lhs._owner, rhs._owner);
        swap(lhs._function_queue, rhs._function_queue);
        swap(lhs._unary_queue, rhs._unary_queue);
        swap(lhs._binary_queue, lhs._binary_queue);
    }

    virtual ~MatrixLazyEvaluator() 
    { // Do absolutely nothing
    }

    using Un_Func = std::function<T(const T&)>;
    using Bin_Func = std::function<T(const T&, const T&)>;
    using Bin_Ex_Func = std::pair<Bin_Func, std::shared_ptr<Matrix<T, container>>>;

    void push_unary(Un_Func func);
    void push_binary(Bin_Func func, Matrix<T, container> operand);

    void execute_next_unary();
    void execute_next_binary();


    void apply_all_changes() noexcept;



private:
    Matrix<T, container>* _owner;

    std::queue<Un_Func>     _unary_queue;
    std::queue<Bin_Ex_Func> _binary_queue;

    enum FunctionType { UNARY, BINARY };
    std::queue<FunctionType> _function_queue;


    template <class X>
    static X pop_queue(std::queue<X>& qe);
};

template <class T, class container>
void MatrixLazyEvaluator<T, container>::push_unary(Un_Func func)
{
    _function_queue.push(UNARY);
    _unary_queue.push(func);
}

template <class T, class container>
void MatrixLazyEvaluator<T, container>::push_binary(Bin_Func func, Matrix<T, container> operand)
{
    _function_queue.push(BINARY);
    _binary_queue.push(std::make_pair(func, std::make_shared<Matrix<T, container>>(operand)));
}

template <class T, class container>
void MatrixLazyEvaluator<T, container>::execute_next_unary()
{
    Un_Func func = pop_queue(_unary_queue);
    _owner->write_unary(func);
}

template <class T, class container>
void MatrixLazyEvaluator<T, container>::execute_next_binary()
{
    Bin_Ex_Func func_struct = pop_queue(_binary_queue);
    _owner->write_binary(func_struct.first, *func_struct.second);
}

template <class T, class container>
template <class X>
X MatrixLazyEvaluator<T, container>::pop_queue(std::queue<X>& qe)
{
    auto val = qe.front();
    qe.pop();
    return val;
}

template <class T, class container>
void MatrixLazyEvaluator<T, container>::apply_all_changes() noexcept
{
    _owner->copy_on_write();
    while (!_function_queue.empty())
    {
        auto ft = pop_queue(_function_queue);
        switch (ft)
        {
        case UNARY: execute_next_unary(); break;
        case BINARY: execute_next_binary(); break;
        default: break;
        }
    }
}
}

RCObject.hpp

    #pragma once
class RCObject
{

public:
    RCObject();
    RCObject(const RCObject& rhs);
    RCObject& operator=(const RCObject& rhs);

virtual ~RCObject() = 0;

void addReference();
void removeReference();

void markUnshareable();
bool isShareable() const;

bool isShared() const;

private:
    size_t ref_count;
    bool shareable;
};

RCObject.cpp

#include "RCObject.hpp"



RCObject::RCObject() :
    ref_count(0), shareable(true)
{
}

RCObject::RCObject(const RCObject& rhs) :
    ref_count(0), shareable(true)
{
}

RCObject& RCObject::operator=(const RCObject& rhs)
{
    return *this;
}

RCObject::~RCObject()
{
}

void RCObject::addReference()
{
    ++ref_count;
}

void RCObject::removeReference()
{
    if (--ref_count == 0) { delete this; }
}

void RCObject::markUnshareable()
{
    shareable = false;
}

bool RCObject::isShareable() const
{
    return shareable;
}

bool RCObject::isShared() const
{
    return ref_count > 1;
}

RCPtr.hpp

#pragma once

template <class T>
class RCPtr
{
public:
    RCPtr(T* real_ptr = nullptr);
    RCPtr(const RCPtr& rhs);
    ~RCPtr();

    RCPtr& operator=(const RCPtr& rhs);

    T* operator->() const;
    T& operator* () const;

private:
    T* _pointee;

    void init();
};

template <class T>
RCPtr<T>::RCPtr(T* real_ptr) :
    _pointee(real_ptr)
{
    init();
}

template <class T>
RCPtr<T>::RCPtr(const RCPtr& rhs) :
    _pointee(rhs._pointee)
{
    init();
}

template <class T>
RCPtr<T>::~RCPtr()
{
    if (_pointee) { _pointee->removeReference(); }
}

template <class T>
RCPtr<T>& RCPtr<T>::operator=(const RCPtr& rhs)
{
    if (_pointee != rhs._pointee)
    {
        T* oldpointee = _pointee;

        _pointee = rhs._pointee;
        init();

        if (oldpointee)
        {
            oldpointee->removeReference();
        }
    }

    return *this;
}

template <class T>
T* RCPtr<T>::operator->() const
{
    return _pointee;
}

template <class T>
T& RCPtr<T>::operator*() const
{
    return *_pointee;
}

template <class T>
void RCPtr<T>::init()
{
    if (_pointee == nullptr) { return; }

    if (!_pointee->isShareable())
    {
        _pointee = new T(*_pointee);
    }

    _pointee->addReference();
}

MatrixStorage.hpp

#pragma once

#include "Array_Wrapper.hpp"


namespace MML
{

    template <class T, class container = Array_Wrapper<T>>
    class MatrixStorage
    {
    public:

        // Constructors and assignments START

        explicit MatrixStorage(const container& arr) noexcept :
            _arr(arr) {}

        MatrixStorage(const MatrixStorage& other) noexcept :
            _arr(other._arr) {}

        MatrixStorage(MatrixStorage&& other) noexcept :
            _arr(std::move(other._arr)) {}

        MatrixStorage& operator=(const MatrixStorage& storage);
        MatrixStorage& operator=(MatrixStorage&& storage);

        virtual MatrixStorage* clone() const = 0;

        // END

        virtual ~MatrixStorage() noexcept{};

        // Operations START

        virtual size_t get_number_of_rows()    const = 0; // READ
        virtual size_t get_number_of_columns() const = 0; // READ

        virtual const T& operator[](const size_t& i) const;  // READ
        virtual       T& operator[](const size_t& i);        // WRITE

        virtual const T& at(const size_t& i, const size_t& j) const; // READ
        virtual       T& at(const size_t& i, const size_t& j);       // WRITE




        void set(const size_t& i, const T& val);                  // WRITE
        void set(const size_t& i, const size_t& j, const T& val); // WRITE

        // Operations END

        // Iterators START 

        using        iterator = typename container::iterator;
        using  const_iterator = typename container::const_iterator;

              iterator  begin(); // WRITE
        const_iterator cbegin(); // READ
              iterator    end(); // WRITE
        const_iterator   cend(); // READ

              iterator  begin() const; // WRITE
        const_iterator cbegin() const; // READ
              iterator    end() const; // WRITE
        const_iterator   cend() const; // READ

        // Iterators END

    protected:
        container _arr;

    };

    template <class T, class container>
    MatrixStorage<T, container>& MatrixStorage<T, container>::operator=(const MatrixStorage& storage)
    {
        *this(storage);
        return *this;
    }

    template <class T, class container>
    MatrixStorage<T, container>& MatrixStorage<T, container>::operator=(MatrixStorage&& storage)
    {
        *this(storage);
        return *this;
    }

    template <class T, class container>
    const T& MatrixStorage<T, container>::operator[](const size_t& i) const
    {
        return _arr[i];
    }

    template <class T, class container>
    T& MatrixStorage<T, container>::operator[](const size_t& i)
    {
        return _arr[i];
    }

    template <class T, class container>
    const T& MatrixStorage<T, container>::at(const size_t& i, const size_t& j) const
    {
        return _arr[i * get_number_of_rows() + j];
    }

    template <class T, class container>
    T& MatrixStorage<T, container>::at(const size_t& i, const size_t& j)
    {
        return _arr[i * get_number_of_rows() + j];
    }

    template <class T, class container>
    void MatrixStorage<T, container>::set(const size_t& i, const T& val)
    {
        operator[](i) = val;
    }

    template <class T, class container>
    void MatrixStorage<T, container>::set(const size_t& i, const size_t& j, const T& val)
    {
        at(i, j) = val;
    }

    template <class T, class container>
    typename MatrixStorage<T, container>::iterator MatrixStorage<T, container>::begin()
    {
        using std::begin;
        return begin(_arr);
    }


    template <class T, class container>
    typename MatrixStorage<T, container>::const_iterator MatrixStorage<T, container>::cbegin()
    {
        using std::cbegin;
        return cbegin(_arr);
    }

    template <class T, class container>
    typename MatrixStorage<T, container>::iterator MatrixStorage<T, container>::end()
    {
        using std::end;
        return end(_arr);
    }

    template <class T, class container>
    typename MatrixStorage<T, container>::const_iterator MatrixStorage<T, container>::cend()
    {
        using std::cend;
        return cend(_arr);
    }

    template <class T, class container>
    typename MatrixStorage<T, container>::iterator MatrixStorage<T, container>::begin() const
    {
        using std::begin;
        return begin(_arr);
    }

    template <class T, class container>
    typename MatrixStorage<T, container>::const_iterator MatrixStorage<T, container>::cbegin() const
    {
        using std::cbegin;
        return cbegin(_arr);
    }

    template <class T, class container>
    typename MatrixStorage<T, container>::iterator MatrixStorage<T, container>::end() const
    {
        using std::end;
        return end(_arr);
    }

    template <class T, class container>
    typename MatrixStorage<T, container>::const_iterator MatrixStorage<T, container>::cend() const
    {
        using std::cend;
        return cend(_arr);
    }

    // Non member functions START

    template <class T, class container>
    typename MatrixStorage<T, container>::iterator begin(const MatrixStorage<T, container>& storage)
    {
        return storage.begin();
    }

    template <class T, class container>
    typename MatrixStorage<T, container>::const_iterator cbegin(const MatrixStorage<T, container>& storage)
    {
        return storage.cbegin();
    }

    template <class T, class container>
    typename MatrixStorage<T, container>::iterator end(const MatrixStorage<T, container>& storage)
    {
        return storage.end();
    }

    template <class T, class container>
    typename MatrixStorage<T, container>::iterator cend(const MatrixStorage<T, container>& storage)
    {
        return storage.cend();
    }

    // Non member functions END
}

DynamicMatrixStorage.hpp

#pragma once

#include  "MatrixStorage.hpp"
#include <algorithm>

namespace MML
{

    template <class T>
    class DynamicMatrixStorage : public MatrixStorage<T>
    {
    public:
        explicit DynamicMatrixStorage(const size_t& rows, const size_t& cols, const Array_Wrapper<T>& arr) noexcept :
            MatrixStorage<T>(arr), _rows(rows), _cols(cols) {}

        DynamicMatrixStorage(const DynamicMatrixStorage& other) noexcept :
            MatrixStorage<T>(other), _rows(other._rows), _cols(other._cols) {}

        DynamicMatrixStorage(DynamicMatrixStorage&& other) noexcept :
            MatrixStorage<T>(other), _rows(other._rows), _cols(other._cols) {}

        ~DynamicMatrixStorage() noexcept {}

        DynamicMatrixStorage* clone() const override
        {
            return new DynamicMatrixStorage(*this);
        }

        size_t get_number_of_rows()    const override;
        size_t get_number_of_columns() const override;

        using       iterator = typename MatrixStorage<T>::iterator;
        using const_iterator = typename MatrixStorage<T>::const_iterator;

        void resize(const size_t& new_rows, const size_t& new_columns);

    private:
        size_t _rows;
        size_t _cols;
    };


    template<class T>
    size_t DynamicMatrixStorage<T>::get_number_of_rows() const
    {
        return _rows;
    }

    template<class T>
    size_t DynamicMatrixStorage<T>::get_number_of_columns() const
    {
        return _cols;
    }

    template <class T>
    void DynamicMatrixStorage<T>::resize(const size_t& new_rows, const size_t& new_columns)
    {
        auto new_size = new_rows*new_columns;
        Array_Wrapper<T> new_storage{ new_size };

        auto old_size = get_number_of_rows() * get_number_of_columns();

        if (new_size >= old_size)
        {
            std::copy(MatrixStorage<T>::_arr.begin(), MatrixStorage<T>::_arr.end(), new_storage.begin());
        }
        else
        {
            std::copy_n(MatrixStorage<T>::_arr.begin(), new_size, new_storage.begin());
        }

        _rows = new_rows;
        _cols = new_columns;
    }
}

StaticMatrixStorage.hpp

#pragma once
#include "MatrixStorage.hpp"


namespace MML
{
    template <class T, size_t rows, size_t cols>
    class StaticMatrixStorage :
        public MatrixStorage<T>
    {
    public:

        explicit StaticMatrixStorage(const Array_Wrapper<T>& arr = Array_Wrapper<T>(rows*cols)) noexcept :
            MatrixStorage<T>(arr) {}

        StaticMatrixStorage(const StaticMatrixStorage& storage) noexcept :
            MatrixStorage<T>(storage) {}

        StaticMatrixStorage(StaticMatrixStorage&& storage) noexcept:
            MatrixStorage<T>(storage) {}

        ~StaticMatrixStorage() noexcept {}

        StaticMatrixStorage* clone() const override
        {
            return new StaticMatrixStorage(*this);
        }

        constexpr size_t get_number_of_rows()    const override;
        constexpr size_t get_number_of_columns() const override;
    };

    template <class T, size_t rows, size_t cols>
    constexpr size_t StaticMatrixStorage<T, rows, cols>::get_number_of_rows() const
    {
        return rows;
    }

    template <class T, size_t rows, size_t cols>
    constexpr size_t StaticMatrixStorage<T, rows, cols>::get_number_of_columns() const
    {
        return cols;
    }
}

Array_Wrapper.hpp

#pragma once

#include <memory>
#include <stdexcept>


namespace MML
{

    // Stores an array, by default creates it on the heap
    template <class T>
    class Array_Wrapper
    {
        std::shared_ptr<T> _arr;
        size_t _size;
    public:
        using       iterator =       T*;
        using const_iterator = const T*;

        explicit Array_Wrapper(const size_t& size): _arr(new T[size]), _size(size) {}
        Array_Wrapper(T* arr, const size_t& size) : _arr(arr),         _size(size) {}
        template <size_t size>
        explicit Array_Wrapper(T arr[size]) :       _arr(arr),         _size(size) {}

        Array_Wrapper& operator=(const Array_Wrapper& other)
        {
            _arr = other._arr;
            _size = other._size;

            return *this;
        }

              T& operator[](const size_t& i)       { return checked_access(i); }
        const T& operator[](const size_t& i) const { return checked_access(i); }


              iterator  begin() { return _arr.get(); }
        const_iterator cbegin() { return _arr.get(); }
              iterator    end() { return _arr.get() + _size; }
        const_iterator   cend() { return _arr.get() + _size; }

    private:
        T& checked_access(const size_t& i) const
        {
            if (i < _size) { return _arr.get()[i]; }
            throw std::out_of_range("Invalid Out of Range Access in Array_Wrapper<T>");
        }
    };

    template <class T>
    typename Array_Wrapper<T>::iterator begin(Array_Wrapper<T>& wrap)
    {
        return wrap.begin();
    }

    template <class T>
    typename Array_Wrapper<T>::const_iterator cbegin(Array_Wrapper<T>& wrap)
    {
        return wrap.cbegin();
    }

    template <class T>
    typename Array_Wrapper<T>::iterator end(Array_Wrapper<T>& wrap)
    {
        return wrap.end();
    }

    template <class T>
    typename Array_Wrapper<T>::const_iterator cend(Array_Wrapper<T>& wrap)
    {
        return wrap.cend();
    }

}

main.cpp

#include <iostream>

#include "MatrixFactory.hpp"
#include <vector>

using namespace MML;

Matrix<int> basic_matrix()
{
    return MatrixFactory<int>::initialise_matrix(10, 10, [](const size_t& row, const size_t& col) { return 10 * row + col; });
}

void matrix_copying()
{
    std::cout << "Testing copies \n\n";

    auto a = basic_matrix();

    std::cout << a;

    auto b{ a };

    std::cout << b;

    auto c = a;

    std::cout << c;

    std::cout << "Copies Testing end\n\n";
}

void matrix_read()
{
    std::cout << "Testing Read Access\n\n";

    auto a = basic_matrix();
    std::cout << "Row 0, Index 0: " << a.at(0, 0) << " Should be 0 \n\n";
    std::cout << "Row 3, Index 5: " << a.at(3, 5) << " Should be 35 \n\n";
    std::cout << "Row 8, Index 7: " << a.at(8, 7) << " Should be 87 \n\n";

    try
    {
        std::cout << "Out of range: " << a.at(10, 10) << " \n\n";
        std::cout << "It hasn't failed yet though\n\n";
    }
    catch (std::out_of_range&)
    {
        std::cout << "It has successfully failed\n\n";
    }

    std::cout << "Read Access Testing end\n\n";
}

void matrix_addition()
{
    std::cout << "Testing addition\n\n";

    auto a = basic_matrix();
    auto b = basic_matrix();
    auto c = basic_matrix();

    std::cout << (b += c);
    std::cout << b;
    std::cout << c;

    // This throws because of a const_violation need to redesign Matrix
    std::cout << a + c;
    std::cout << a;
    std::cout << c;

    std::cout << "Addition Testing end\n\n";
}

void matrix_subtraction()
{
    std::cout << "Testing subtraction\n\n";

    auto a = basic_matrix();
    auto b = basic_matrix();
    auto c = basic_matrix();

    std::cout << (b -= c);
    std::cout << b;
    std::cout << c;

    // This throws because of a const_violation need to redesign Matrix
    std::cout << a - c;
    std::cout << a;
    std::cout << c;

    std::cout << "Subtraction Testing end\n\n";
}

void matrix_scalar_multiplication()
{
    std::cout << "Testing Scalar Multiplication\n\n";

    auto a = basic_matrix();

    std::cout << (a *= 3);
    std::cout << a;

    // This throws because of a const_violation need to redesign Matrix
    auto b = basic_matrix();
    std::cout << b * 3;
    std::cout << 3 * b;
    std::cout << b;

    std::cout << "Scalar Multiplication Testing end\n\n";
}

void matrix_scalar_division()
{
    std::cout << "Testing Scalar Division\n\n";

    auto a = basic_matrix();

    std::cout << (a /= 3);
    std::cout << a;

    // This throws because of a const_violation need to redesign Matrix
    auto b = basic_matrix();
    std::cout << b / 3;
    std::cout << b;

    std::cout << "Scalar Division Testing end\n\n";
}

void matrix_equality()
{
    std::cout << "Testing Equality\n\n";

    auto a = basic_matrix();
    auto b = basic_matrix();
    auto c = a;

    std::cout << "Is a == b: " << (a == b) << "\n";
    std::cout << "Is a == c: " << (a == b) << "\n";
    b *= 2;
    std::cout << "Is a == modified b? " << (a == b) << " \n";

    std::cout << "Equality Testing end\n\n";
}



int main(int argc, char* argv[])
{
    matrix_copying();
    matrix_read();
    matrix_addition();
    matrix_subtraction();
    matrix_scalar_multiplication();
    matrix_scalar_division();
    matrix_equality();
}
\$\endgroup\$
  • \$\begingroup\$ Ownership semantics jumps out. You are passing pointers around with indicating ownership. \$\endgroup\$ – Martin York Jan 10 '17 at 18:51
4
\$\begingroup\$

Here are my thoughts:

  • Even though #pragma once is support by every major compiler (VC, gcc, clang and icc), it is a language extension. You should try to write the most standard compliant code possible, so #ifndef guards would be better.

  • Your code doesn't compile for me with gcc. That's because (1) you are using std::size_t without including a header that defines it (like <cstddef>) and (2) std::size_t is in std, not in the global namespace.

  • Fixing the previous errors, your code still doesn't compile, and that's because you cannot use parameters of a function for default arguments of the same function:

    void foo(int a, int = a); // illegal
    

    To fix this, define a second function:

    void foo(int a) { foo(a, a); }
    void foo(int a, int) { /* ... */ }
    
  • That's probably VS's fault, but gcc emits this warning:

    warning: base class 'class RCObject' should be explicitly initialized in the copy constructor [-Wextra]
         MatrixValue(const MatrixValue& rhs) :
    

    Always initialize the base class in the derived class's constructor.

  • Always make your code compile with no warnings on high (but not aggressive) levels of warnings. You have a lot of parameters that you don't use, i.e. argc, argv and rhs. Consider omitting their names or in the case of main, just remove them.

  • You are not using the copy and swap idiom.

  • A destructor who does nothing should be defaulted:

    virtual RCObject() = default;
    
  • Use auto to save a lot of characters!

    template <class T, class container>
    typename MatrixStorage<T, container>::iterator MatrixStorage<T, container>::end()
    {
        using std::end;
        return end(_arr);
    }
    

    becomes

    template <class T, class container>
    auto MatrixStorage<T, container>::end()
    {
        using std::end;
        return end(_arr);
    }
    

    way more readable!

  • Returning mutable iterators from a const container is not good and can lead to undefined behavior. The begin and end functions shouldn't be marked const, and the cbegin and cend should be marked const. Not some other permutations of constness.

  • I don't get the point of Array_Pointer. Isn't a std::vector enough? It is an array allocated on the heap, just like you want.

  • In some classes, you are not implementing the Rule of Five.

  • Again, why did you make your own shared pointer? Why not use std::shared_ptr? It works flawlessly with only 1 modification to your code

  • RCPtr is useless (you're not using it anywhere), you can remove it safely.

  • Declare your classes as final if they shouldn't be derived from.

  • Don't use new/delete please. Instead, use std::unique_ptr and co.

  • I think it would be better if you hid the storage inside of Matrix, and implemented everything using the constructors, without any factory. To specify the storage type, use the Policy Pattern, the same that you used for the container.

    I don't like factories, for the only reason that in my opinion they are completely unnecessary. You can just use the constructor of the class the factory creates and that's it. But in your case, that's not possible, because the user has to pass a storage object, fully initialized. But why does the user have to do this? This is supposed to be a library detail, which only Matrix should have access to (and should own), not the user. This leads to all kinds of problems.

    But there still has to be a way to specify the storage type, right? Yes, this is where the policy pattern kicks in. Actually, you have already done it with the container parameter: The policy pattern just states that you can modify a class as you please (something like that). By passing template parameters, you can choose which behavior you want the class to have for different policies. Which container? How are the object allocated? How are they stored? Those things can be customized using policies:

    template<typename StoragePolicy, typename ContainerPolicy = /*...*/>
    class Matrix {};
    

    That way, the user can choose which storage they want, without actually needing to initialize it, that's Matrix's job.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback! I am not 100% clear on the last point tough, could you slightly clarify it please? \$\endgroup\$ – WizardOfMenlo Jan 11 '17 at 21:02
  • 1
    \$\begingroup\$ @WizardOfMenlo There you go, I hope it's more clear now. \$\endgroup\$ – Rakete1111 Jan 11 '17 at 22:21

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.