delete[] triggers breakpoint on return from function

Question

I wrote a matrix class that can do certain matrix operations, but my newest function, transpose(), triggers a breakpoint on delete[]. Here are the relevant parts:

class Matrix {
    float* M;
    int n, k;
    void allocM(int _n, int _k) {
        n = _n;
        k = _k;
        M = new float[n * k];
    }
    void delM() {
        delete[] M;
    }
    public:
    inline float operator()(int i, int j) const { return M[i * k + j]; }
    inline float& operator()(int i, int j) { return M[i * k + j]; }
    Matrix(int _n, int _k) {
        allocM(_n, _k);
        for (int i = 0; i < n; ++i) {
            for (int j = 0; j < k; ++j) {
                operator()(i, j) = 0;
            }
        }
    }
    Matrix(const Matrix& source) {
        allocM(source.n, source.k);
        for (int i = 0; i < n; ++i) {
            for (int j = 0; j < k; ++j) {
                operator()(i, j) = source(i, j);
            }
        }
    }
    ~Matrix() {
        delM();
    }
    Matrix& operator=(const Matrix& source) {
        delM();
        allocM(source.n, source.k);
        for (int i = 0; i < n; ++i) {
            for (int j = 0; j < k; ++j) {
                operator()(i, j) = source(i, j);
            }
        }
        return *this;
    }
    Matrix operator*(const Matrix& other) const {
        Matrix matrix = Matrix(n, other.k);
        for (int i = 0; i < matrix.n; ++i) {
            for (int j = 0; j < matrix.k; ++j) {
                for (int l = 0; l < k; ++l) {
                    matrix(i, j) += operator()(i, l) * other(l, j);
                }
            }
        }
        return matrix;
    }
    Matrix transpose() const {
        Matrix matrix = Matrix(k, n);
        for (int i = 0; i < n; ++i) {
            for (int j = 0; j < k; ++j) {
                matrix(k, n) = operator()(n, k);
            }
        }
        return matrix;
    }
};

I then multiply two matrices, basically. Something like:

matrix0 = matrix1 * matrix2.transpose();

Result: a breakpoint is triggered in delM() { delete[] M; }

I added a breakpoint to the matrix multiplication. This is what happens:

Program enters transpose() function, then Matrix(int, int) constructor runs. Then the nested loops run in transpose(), followed by return matrix;, where it runs Matrix(const Matrix& source), then back to return matrix;, then delM() and then it breaks on the line delete[] M;

I have only included the assignment operator function to show why I have a delM() function. I have tried zeroing *M out after declaration and in delM() after delete, but it didn't help.

What am I doing wrong?

Answer 1

In transpose, you should change:

matrix(k, n) = operator()(n, k);

To:

matrix(j, i) = operator()(i, j);

You're accessing data off the end of the array M, causing heap corruption. It may be a good idea to do a bounds check in operator(), just to catch these kinds of things in the future.

And, as I mentioned in a comment, in operator=, add:

if (this == &source) return;

Or preferably (as Puppy points out), design it such that self-assignment is safe. Crude example:

    float *temp = M;
    allocM(source.n, source.k);
    for (int i = 0; i < n; ++i) {
        for (int j = 0; j < k; ++j) {
            operator()(i, j) = source(i, j);
        }
    }
    delete []temp;
    return *this;

Answer 2

Your assignment operator is broken, along with every use of new and delete in your class. Use std::vector<float>, it has already solved all these problems for you in a far more efficient and correct way.

Answer 3

Your assignment operator, given that your copy constructor and destructor are working perfectly, can be written like this:

Matrix& operator=(Matrix source) 
{
   std::swap(source.n, n);
   std::swap(source.k, k);
   std::swap(source.M, M);
   return *this;
}

There is no need for a check for self-assignment, and it is exception safe. Your assignment operator you had written screws up the original object if new[] throws an exception. The code above alleviates it.

The code above uses the copy/swap idiom. If you follow the code, you will see how it works, and why it is an ingenious way of implementing an assignment operator given that the copy constructor and destructor are working.

The synopsis is that you create a temporary Matrix from the passed in argument, and then swap out what you created with the current (this) data. Then you let the temporary die off with the "old stuff", while the "new stuff" was given to this (that's what all the swapping is about).

The std::swap just swaps the two values with each other. If your requirements are so draconian that you can't call a std::swap function, then write your own and just call it, or write it "inline":

template <typename T>
void mySwap(T& x, T& y)
{
   T temp = x;
   x = y;
   y = temp;
}

Matrix& operator=(Matrix source) 
{
   mySwap(source.n, n);
   mySwap(source.k, k);
   mySwap(source.M, M);
   return *this;
}

Edit: Changed signature to allow compiler to make initial copy of the passed-in object.