CODEWITHSUNDEEP
c++
Sergei Gorskiy
Sergei Gorskiy

Reputation: 13

How to improve "=" operator overload for matrices?

I have overloaded assignment operator for the class with a 2D array, but in order to do memory management and resizing correct I have to delete previous matrix first, then construct a new one, and only then I can start assigning.

Matrix& Matrix::operator = (const Matrix& m1){
    for (int i = 0; i < m_rows; ++i)
        delete[] m_matrix[i];
    delete[] m_matrix;

    m_matrix = new double*[m1.rows()];
    for (int i = 0; i < m1.rows(); ++i)
        m_matrix[i] = new double[m1.cols()]();

    for (int k = 0; k < m1.rows(); ++k)
        for (int j = 0; j < m1.cols(); ++j)
            m_matrix[k][j] = m1.m_matrix[k][j];

    m_rows = m1.rows();
    m_cols = m1.cols();

    return *this;
}

In fact, this part is destructor of my class: 

for (int i = 0; i < m_rows; ++i)
    delete[] m_matrix[i];
delete[] m_matrix;

And this part is similar to a constructor:

m_matrix = new double*[m1.rows()];
for (int i = 0; i < m_rows; ++i)
    m_matrix[i] = new double[m1.cols()]();

What annoys me is that I have to copy constructors' and destructors' code in the assignment function (and some other functions too!) to make it work properly. Is there a better way to write it?

Upvotes: 0

Views: 115

Answers (2)

Maxim Egorushkin
Maxim Egorushkin

Reputation: 136306

The ideal improvement would be Matrix& Matrix::operator=(const Matrix&) = default;.

If you switch to using std::vector for matrix storage you won't need to implement the copy/move constructors/assignments and destructor at all.

If what you are doing is a programming exercise, create your own dynamic array and use that in the implementation of your matrix.

I cannot recommend enough watching Better Code: Runtime Polymorphism by Sean Parent, he makes an effective demonstration of why you should strive to write classes that do not require non-default implementations of copy/move constructors/assignments and destructor.

Example:

template<class T>
class Matrix
{
    std::vector<T> storage_;
    unsigned cols_ = 0;

public:
    Matrix(unsigned rows, unsigned cols)
        : storage_(rows * cols)
        , cols_(cols)
    {}

    // Because of the user-defined constructor above
    // the default constructor must be provided.
    // The default implementation is sufficient.
    Matrix() = default;

    unsigned columns() const { return cols_; }
    unsigned rows() const { return storage_.size() / cols_; }

    // Using operator() for indexing because [] can only take one argument.
    T& operator()(unsigned row, unsigned col) { return storage_[row * cols_ + col]; }
    T const& operator()(unsigned row, unsigned col) const { return storage_[row * cols_ + col]; }

    // Canonical swap member function.
    void swap(Matrix& b) {
        using std::swap;
        swap(storage_, b.storage_);
        swap(cols_, b.cols_);
    }

    // Canonical swap function. Friend name injection.
    friend void swap(Matrix& a, Matrix& b) { a.swap(b); }

    // This is what the compiler does for you, 
    // not necessary to declare these at all.
    Matrix(Matrix const&) = default;
    Matrix(Matrix&&) = default;
    Matrix& operator=(Matrix const&) = default;
    Matrix& operator=(Matrix&&) = default;
    ~Matrix() = default;
};

Upvotes: 3

Dietmar K&#252;hl
Dietmar K&#252;hl

Reputation: 153895

The canonical implementation of the assignment operator leverages existing functionality (copy/move ctor, dtor, andswap(); note that using a non-specialized std::swap() would be bad). It looks like this:

T& T::operator= (T val) {
    val.swap(*this);
    return *this;
}

It nicely avoids reimplementing otherwise existing logic. It also deals gracefully with self-assignment which is a problem in your original code (it will do work but self-assignment is generally rather uncommon; optimizing it with a check against self-assignment typically pessimizes code).

The argument is passed by value to take advantage of copy elision.

The primary caveats with this approach are below. In general I prefer the canonical implementation as it is generally more correct and the outlined issue are often not really that relevant (e.g., when the object was just created anyway the transferred memory is actually "hot").

  1. It does not attempt to reuse already allocated and possibly "hot" memory. Instead it always uses new memory.
  2. If the amount of held data is huge, there are copies temporarily held which may exceed system limits. Reusing existing memory and/or release memory first would both address this issue.

Upvotes: 1

Related Questions