c++ allocation and destructor

Question

I got this class :

int x;
int y;
int **mat;

MyMatrix::MyMatrix(int a, int b)
    :x(a), y(b)
{
    int i ,j;
    mat = new int*[x];
    for (int i = 0; i < x; ++i)
        mat[i] = new int[y];

    for (i = 0; i < x; ++i){
        for (j = 0; j < y; ++j){
            mat[i][j] = i + j;
        }
    }
}

MyMatrix& MyMatrix::add(MyMatrix m){
    int i, j;

    if (x != m.x || y != m.y) return *this;

    for (i = 0; i < x; ++i){
        for (j = 0; j < y; ++j){
            mat[i][j] += m.mat[i][j];
        }
    }

    return *this;
}

MyMatrix::~MyMatrix()
{
    for (int i = 0; i < x; ++i)
        delete[] mat[i];

    delete mat;
}

and this is my main :

int main(){
    MyMatrix m(2, 3);
    MyMatrix m1(2, 3);

    m.add(m1);

    m.print();
}

All is working and prints me the right answer, but there is some allocation problem. I'm working with debugger and see that the program goes twice to the destructor, on the second time the program crushes.

Please explain me whats the problem and why ?

The allocation of the two matrixes where fine. After the adding when I got to return it goes to destructor. and on the end of main it goes to destructor again and crushes there — igor, Dec 05 '14 at 16:08
You probably haven't followed the [Rule of Three](http://stackoverflow.com/questions/4172722). Combined with passing by value, not reference, to `add`, your class is a primed grenade, ready to explode at any time. I'd replace that pointer-juggling with `std::vector>` to sort out all the memory management for me. — Mike Seymour, Dec 05 '14 at 16:09
Or, just use an `std::vector` and do the offset math in the accessor. (This will require less memory and fewer levels of indirection.) — cdhowie, Dec 05 '14 at 16:11

score 2 · Accepted Answer · answered Dec 05 '14 at 16:07

2

Yes when you call m.add the parameter is passed by value so a new matrix is built at the entry of the method and then destroyed at the end. To eliminate this effect use a by-ref passing :

MyMatrix& MyMatrix::add(const MyMatrix &m){
...
}

This way the original one is passed into the function, but the const prevents you to modify it during the execution of the method.

answered Dec 05 '14 at 16:07

Jean-Baptiste Yunès

34,548
4
48
69

Alternatively, the OP should follow the rule of 3/5. – Dec 05 '14 at 16:07
It's working thanks. Can you explain more ? why is passing a copy make the program to crush? – igor Dec 05 '14 at 16:11
2

@igor Because the object gets copied. The default copy constructor will copy the data members by value, so it copies the pointers. Suddenly you have two objects where `mat` points to the same array. When the copy is destructed the allocated array is deleted. Now you have the original object with a pointer to a deleted array. It should be clear why this is a problem. If you use `std::vector` instead of raw arrays this won't be a problem, as copying a vector means making a copy of its contents; the copy of the matrix object would have its own copy of the data. – cdhowie Dec 05 '14 at 16:13

score 1 · Answer 2 · edited May 23 '17 at 12:27

The most obvious thing I see is that in your destructor you need to use delete [] on mat. The other problem is that you need to supply a copy-constructor. I believe this answer is on to the biggest problem. When you pass by value and make a copy of MyMatrix, the default copy-constructor just copies all the member variables: x, y, and mat. This makes a copy of the mat pointer, not the data it points to. When that temporary MyMatrix is destroyed, the allocated data from m1 is deleted because the temporary's mat is pointed to it. So when your program tries to destroy m1, it deletes a pointer that has already been deleted and you get a crash.

A copy constructor would look like this:

MyMatrix::MyMatrix(const MyMatrix& other) : x(other.x), y(other.y)
{
    mat = new int*[x];
    for (int i = 0; i < x; ++i)
        mat[i] = new int[y];

    for (int i = 0; i < x; ++i){
        for (int j = 0; j < y; ++j){
            mat[i][j] = other.mat[i][j];
        }
    }
}

A couple other tips. You might want to use operator+= instead of your add function so you can write code like:

m += m1;

Just change MyMatrix& MyMatrix::add(MyMatrix m) to MyMatrix& MyMatrix::operator+=(const MyMatrix &m). Also, your allocation of memory is relatively inefficient. It will work, but there are better ways to implement a 2D matrix. If this is a learning exercise this is fine. If you really want to work with matrices in C++, I recommend looking into a library like OpenCV.

c++ allocation and destructor

2 Answers2