overloaded * operator fails when calling destructor after multiple * operations

Question

I am making a class to do matrix (and vector) math for a test I am running and to learn more C++. The class looks like this:

class utlMatrix
{
private:
    int num_rows;
    int num_cols; // number of columns
    double **data; // array of pointers to the data

public:
    // default constructor, with initialization
    utlMatrix() : num_rows(0), num_cols(0), data(NULL) {};

    // constructor with size
    utlMatrix(int, int);

    // destructor
    ~utlMatrix();

    // copy constructor
    utlMatrix(const utlMatrix&);

    void copy(const utlMatrix &old); // copy 'old' to 'this'
    void zero(); // sets all values to zero
    void fill_rand(); //fills the data with random stuff
    void print(std::ostream&); // prints the matrix to a file

    // Operators
    utlMatrix& operator=(const utlMatrix&); // copies matrices
    friend utlMatrix operator+(const utlMatrix&, const utlMatrix&); // adds 2 matrices
    utlMatrix operator*(const utlMatrix&) const;

    //friend utlMatrix operator*(const utlMatrix&, const utlMatrix&); // multiplies 2 matrices
};

Copy Constructors, assignment operator and destructor

// copy constructor
utlMatrix::utlMatrix(const utlMatrix &old) {
    copy(old);
}
utlMatrix& utlMatrix::operator=(const utlMatrix &old)    {
    copy(old);
    return *this;
}
void utlMatrix::copy(const utlMatrix &old)    {
    num_rows = old.num_rows;
    num_cols = old.num_cols;
    data = new float*[num_rows];
    for (int i = 0; i < num_cols; i++)
        data[i] = new float[num_cols];
    for (int i = 0; i < num_rows; i++)
    {
        for (int j = 0; j < num_cols; j++)
            data[i][j] = old.data[i][j];
    }
}
utlMatrix::~utlMatrix()
{
    for (int i = 0; i < num_rows; i++)
        delete [] data[i];
    delete [] data;
}

Multiplication operators, I tried both, both failed if used twice in a row.

/*
utlMatrix operator*(const utlMatrix &left, const utlMatrix &right)
{
    // first determine if the matrices can be multiplied
    if (left.num_cols != right.num_rows)
    {
        std::cout << "Error using *, Inner dimensions must agree." << std::endl;
        exit(-1);
    }
    // create the new matrix
    utlMatrix newmat(left.num_rows, right.num_cols);
    for (int i = 0; i < left.num_rows; i++)
        for (int j = 0; j < right.num_cols; j++)
            for (int k = 0; k < right.num_rows; k++)
                newmat.data[i][j] += left.data[i][k] * right.data[k][j];
        return newmat;
}
*/
utlMatrix utlMatrix::operator*(const utlMatrix &right) const
{
        if ( this->num_cols != right.num_rows)
    {
        std::cout << "Error using *, Inner dimensions must agree." << std::endl;
        return utlMatrix();
    }
    utlMatrix newmat(this->num_rows, right.num_cols);
    for (int i = 0; i < this->num_rows; i++)
        for (int j = 0; j < right.num_cols; j++)
            for (int k = 0; k < right.num_rows; k++)
                newmat.data[i][j] += this->data[i][k] * right.data[k][j];
    return newmat; 
}

The copy constructor, assignment and addition operators all work fine. When I try to multiply 1 matrix by another it works the first time but fails on the second. I altered the code to write out when it enter a constructor, operator and destructor in addition to printing the matrices after the multiplication. The math is good for the first matrix and the code fails with:

Unhandled exception at 0x776015de in sandbox.exe: 0xC0000005: Access violation writing location 0xcdcdcdcd.

From the screen output I know this is happening after the copy constructor is called following the second multiplication. The addition operator mirrors the first multiplication operator and appears to work fine, no exceptions, the copy constructor and destructor happen as expected. I found 2 answers on SO, Matrix class operator overloading,destructor problem and Matrix Multiplication with operator overloading. I checked to make sure that my pointers were not copied. The copy constructor does create a new object with a new pointer to data. If I run:

int main()
{
utlMatrix A(3,3);
utlMatrix B(2,2);
A.fill_rand();
B.fill_rand();    
B = A;
return 0;
}

The debugger shows:

A   {num_rows=3 num_cols=3 data=0x000365c0 ...} utlMatrix
B   {num_rows=3 num_cols=3 data=0x00037c50 ...} utlMatrix

What did I miss? Here is how I actually used the operator. Failure occurs after D = A * B;

#include "utlMatrix.h"

int main()
{
    // create the first matrices
    utlMatrix A(3,3);
    utlMatrix B(3,3);
    utlMatrix C(3,2);

    // fill them with random numbers
    A.fill_rand();
    B.fill_rand();
    C.fill_rand();

    utlMatrix D = A * B;
    utlMatrix E = A * C;
    utlMatrix F = B * C;
}

In `utlMatrix::copy`, you have `data = new float*[num_rows]; for (int i = 0; i < num_cols; i++) ...`. Replace `num_cols` with `num_rows` — Igor Tandetnik, Aug 07 '13 at 21:23
Also, your assignment operator isn't really "fine", as it leaks since it doesn't delete the previous data. However, that's not the issue in your crash right now. — Dave S, Aug 07 '13 at 21:25
What does `utlMatrix(int, int)` constructor look like? Does it allocate and zero out `data` array? You rely on that in your `operator*` — Igor Tandetnik, Aug 07 '13 at 21:25
`copy` is defined but not declared. Could you post your *actual* code? And maybe even try to minimize it first if you want to save us some trouble? — Beta, Aug 07 '13 at 21:26
@IgorTandetnik Can't believe I missed that. I made that fix and code ran fine. And yes the utlMatrix(int, int) does zero out the matrix. — Matt, Aug 07 '13 at 21:34
@DaveS Can you suggest a fix? As I said I am trying to learn this stuff so any help would be great. — Matt, Aug 07 '13 at 21:40
I recommend the copy-and-swap algorithm: http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom which handles the self assignment case and exceptions all in one go. — Dave S, Aug 07 '13 at 21:43
Your `copy` method initializes `data` with `float`s even though it's declared as an array of pointers to `double`. If your non-copy constructor does the same thing, this could be the source of your errors. — Edward, Aug 07 '13 at 22:38
Also, the copy function has a memory-leak: If you assign a matrix to a existing instance, copy results in a leak (Data ptr is reassigned, But not freed first) — Manu343726, Aug 07 '13 at 22:44
Adding insult to injury, this code can't compile anyway if your platform `double` and `float` are not synonymous. Your member variable `data` is a `double **` and yet you're assigning it `new float*[num_rows];`. The same is true for the actual row allocations, assigning `data[i] = new float[num_cols];` Case in point, this does not compile on my Macbook Air (10.8.3). Further, your class had no mention of `copy` as a prototyped member in the original post. I added it just to assist in getting this up and running. Please post **real** code in the future that *compiles*. — WhozCraig, Aug 07 '13 at 23:41

score 1 · Accepted Answer · answered Aug 28 '13 at 20:30

The error appeared to only show itself after the second or third call because that was when the matrices produced non-square output. These lines:

for (int i = 0; i < num_cols; i++)
    data[i] = new float[num_cols];

in the copy constructor meant the non-square matrix was actually being built as a square matrix of size columns of the old one. Since my case was a matrix with more rows than columns it tried to put data into a non-existing memory location. Following the suggestions of Igor Tandetnik and Dave S, fixing the indexes and using SWAP fixed the problem.

I was just about to say the same thing - only took me a minute to spot it. — Mark Ransom, Aug 28 '13 at 20:34

overloaded * operator fails when calling destructor after multiple * operations

1 Answers1