C++: Implementing move assignment operator on two dimensional array

Question

I have a class meant to implement a matrix, here:

template<typename Comparable>
class Matrix {
    private: 
        std::size_t num_cols_;
        std::size_t num_rows_;
        Comparable **array_;

    public:
        Matrix();
        ~Matrix(); // Destructor
        Matrix(const Matrix<Comparable> & rhs);// Copy constructor
        Matrix(Matrix<Comparable> && rhs); // Move constructor
        Matrix<Comparable> & operator= (const Matrix<Comparable> & rhs);// Copy assignment
        Matrix<Comparable> & operator= (Matrix<Comparable> && rhs); // Move assignment
        template<typename buh> friend std::ostream &operator<< (std::ostream &os, Matrix<buh> &rhs);
        void ReadMatrix();
};

(Vectors aren't an option for this particular problem.)

The array_ member in particular holds the matrix itself, and is populated using the following code:

array_ = new Comparable*[num_rows_];
    for (int i = 0; i < num_rows_; ++i) {
        array_[i] = new Comparable[num_cols_];
    };

    for(int i = 0;i < num_rows_; ++i) {
        std::cout << "Enter items for row " << i << "." << std::endl;
        for(int j = 0;j < num_cols_; ++j) {
            std::cin >> array_[i][j];
        }
    }

I can fill the array with values and access them, and my copy constructor and move assignment operator are functional, but the move assignment operator throws out a strange bug. here's the definition.

template<typename Comparable>
Matrix<Comparable>& Matrix<Comparable>::operator= (Matrix<Comparable> && rhs) {
    delete[] array_;
    array_ = new Comparable*[rhs.num_rows_];    
    for(int i = 0;i < rhs.num_rows_;++i) {
        std::swap(array_[i],rhs.array_[i]);
        rhs.array_[i] = nullptr;
    }
    rhs.num_cols_ = 0;
    rhs.num_rows_ = 0;
    rhs.array_ = nullptr;
    return *this;
}

Take the statement a = std::move(b);. If b is of a different size than a, the matrix data is deformed by the move. If b has more columns than a, the extra columns will be cut off; if b has fewer rows than a, the excess rows will remain in a; if a has more columns or rows than b, the excess columns will display memory address where there should be nothing at all. Is this a simple bug? Is there a problem with way I create the arrays? Any insight into what's causing this is appreciated.

If this isn't homework then the answer is "use a `std::vector>`. — user657267, Sep 11 '15 at 04:35
Your move assignment operator 1) never update `this`'s row and column count, 2) leaks `rhs.array_`, 3) leaks all the rows previously held by `this`, and 4) performs an unnecessary allocation. — T.C., Sep 11 '15 at 05:01
If you can't use `std::vector`, use `std::unique_ptr`, and then use default move constructor. And if you can't use `std`, write your own `vector`/`unique_ptr` before to write `Matrix`. — Jarod42, Sep 11 '15 at 07:59

score 0 · Answer 1 · edited May 23 '17 at 12:03

Not sure why you are using new Comparable* in your move assignment operator. The idea of a move assignment is to move the resources, not make new ones.

Your code could look like:

delete[] array_;
array_ = rhs.array_;
rhs.array_ = nullptr;
num_cols_ = rhs.num_cols_;
num_rows_ = rhs.num_rows_;
return *this;

However, consider using the copy-and-swap idiom. It's not always the most efficient option but it is a good starting point if you're not a guru.

Note: If you really want to use a pointer to pointer to implement your matrix, use vector<vector<Comparable>> instead. All the work is done for you; your code is just reinventing the wheel.

Usually it is simpler and more effective to represent a matrix with one contiguous allocation, instead of a separate allocation for each row, so you may want to give that idea some thought.

score 0 · Accepted Answer · answered Sep 11 '15 at 05:34

0

"Move assign" doesn't mean "carefully modify the passed in object to become some sort of 'empty' value", it means "it's fine to modify the passed in object".

Move assignment here should have a very simple implementation: just swap.

template<typename Comparable>
Matrix<Comparable>& Matrix<Comparable>::operator= (Matrix<Comparable> && rhs) {
    using std::swap;
    swap(array_, rhs.array_);
    swap(num_cols_, rhs.num_cols_);
    swap(num_rows_, rhs.num_rows_);
    return *this;
}

answered Sep 11 '15 at 05:34

1

"it's fine to modify the passed in object" should be qualified with "the object is supposed to be left in a *valid but unspecified* state", or something similar. For example it's *not* fine to do `array = rhs.array_;` without anything else, in case the caller tries to use `rhs` after the move. – M.M Sep 11 '15 at 05:41
really got to resist the urge to overcomplicate these things. Thanks. – user3564783 Sep 11 '15 at 11:16

C++: Implementing move assignment operator on two dimensional array

2 Answers2