C++ Destructor being called in overloaded arithmetic operators

Question

I have a custom-made Matrix library for a neural network program and overloaded arithmetic operators. Here's the class declarations:

class Matrix{
public:
int m;
int n;
double **mat;
Matrix(int,int);
Matrix(int);
Matrix(const Matrix& that):mat(that.mat),m(that.m),n(that.n)
    {
        mat = new double*[m];
        for(int i = 0;i<m;i++)mat[i] = new double[n];
    };
~Matrix();
friend istream& operator>>(istream &in, Matrix &c);
friend ostream& operator<<(ostream &out, Matrix &c);
Matrix operator+(const Matrix& other);
};

This is the function definition for + operation:

 Matrix Matrix::operator+(const Matrix& other)
    {
        Matrix c(m,n);
        for(int i=0;i<m;i++)
        {
           for(int j = 0; j<n;j++)
               c.mat[i][j] = mat[i][j] + other.mat[i][j];
        }
        return c;
    }

I have tried to implement it in all ways and error is same...here's an instance

Matrix x(m,n); //m and n are known
x = a+b; // a and b are also m by n matrices

I have debugged the code using breakpoints and here's the error... The local matrix 'c' in operator function is destroyed before it is returned and hence what is assigned to x is a garbage pointer..

Please suggest me something...

Of course it is destroyed, this is how c++ works. Did you implement a deep copy in your copy constructor and assignment operator? And show your constructor/destructor. — , Feb 21 '13 at 14:47
http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three — Benjamin Lindley, Feb 21 '13 at 14:47
Instead of using `double ** mat` consider using a [vector](http://stackoverflow.com/a/13937325/942596). The advantage here is you can use the trivial copy constructor. — andre, Feb 21 '13 at 14:51

score 2 · Accepted Answer · answered Feb 21 '13 at 14:46

2

You need to define a copy constructor for your class. The copy constructor will need to allocate memory for mat and make a copy of the data.

Without this, when you return c, a new object is constructed that has the same value of mat as c. When c subsequently goes out of scope, it deletes c.mat. As a result, the copy of c is left with a dangling pointer.

Having done this, you should also implement an assignment operator.

answered Feb 21 '13 at 14:46

NPE

486,780
108
951
1,012

Thanks NPE...I did define a copy constructor! But can you please tell how to edit my operator function code? Thanks in advance! – Cheeku Feb 21 '13 at 15:03
@Cheeku: If your constructors (including your copy constructor) and your destructor are all implemented correctly, I see nothing wrong with your operator, although it should probably be a const member function. – Benjamin Lindley Feb 21 '13 at 15:08
@BenjaminLindley Well, I did modify the code with a copy constructor. I edited the code in my question to show that. But it doesn't work as expected! What could be the possible errors? – Cheeku Feb 21 '13 at 15:13

Andy Prowl · Answer 2 · 2013-02-21T14:57:40.827

The value you returned is used to initialize a temporary, and this temporary is then copied into the result after the value you returned has been destroyed. This is normal behavior (unless the call is elided because of NRVO).

However, since your class has no explicitly defined copy constructor, the implicitly generated one will be invoked, and that will just copy a pointer (mat) to stuff that has been deallocated by the returned object's destructor.

This is a violation of the so-called Rule of Three, a programming best-practice saying that whenever your class explicitly defines a copy-constructor, an assignment operator, or a destructor, then it should define all of them. The rationale is that a class that defines one of them most likely does so because it is managing some resource, and for correctly handling resource releasing/acquiring logic, all of those three special member functions are needed.

Notice that in C++11 you can also have a move constructor that will be allowed to perform the transfer of the Matrix's content just by assigning pointers and invalidating the object you moved from.

Matrix(Matrix&& m)
{
    mat = m.mat;
    m.mat = nullptr;
}

Of course, if you introduce a move constructor, you will have to modify your class destructor accordingly to check if you really have to release the allocated memory:

~Matrix()
{
    if (m.mat == nullptr)
    {
        return;
    }

    ...
}

Thanks, copy constructor is what I didn't know about! – Cheeku Feb 21 '13 at 14:50 — Cheeku, Feb 21 '13 at 14:50

score 0 · Answer 3 · answered Feb 21 '13 at 14:47

Your Matrix class has a raw pointer member and presumably allocates memory in its constructors, yet you have no copy constructor or copy assignment operator.

Also, you have a destructor, yet you have no copy constructor or copy assignment operator. This is a violation of the Rule of Three.

Al Kepp · Answer 4 · 2013-02-21T14:50:40.977

Your Matrix c is a local variable. So it is destroyed when that method where it was created ends. In C++ this unwanted situation is usually solved by copying objects. You can define copy constructor and assignment operator = with the same functionality. The problem of copying is that it is slow, so if you want it to be faster, you should use a different approach withotu copying. For example, you can add a parameter to the method where caller would pass a reference to the existing matrix object where to store the result.

score 0 · Answer 5 · answered Feb 21 '13 at 14:51

You need a copy constructor and an assignment operator for your class that make a deep copy of the object as the compiler generated functions will not.

The compiler generated copy constructors and assignment operators will simply copy the objects contained in the class. In your case these are PODs, so the automatically generated functions will simply do a bitwise copy. In the case of the double**, this will result in the copy of the pointer value, not the pointed to values. As a result, you end up with two Matrix objects pointing to the same underlying data, just before the destructor pulls the rug out from underneath you.

MrHug · Answer 6 · 2013-02-21T19:24:23.020

-1

You should change your code to return a Matrix *, rather than a Matrix object. This way you can ensure that the Matrix object lives after the function. (Your current code makes the Matrix object a function variable, thus it will be removed after the function has ended).

Your code could look like this:

Matrix *Matrix::operator+(const Matrix& other)
{
    Matrix *c = new Matrix(m,n);
    for(int i=0;i<m;i++)
    {
       for(int j = 0; j<n;j++)
           c->mat[i][j] = mat[i][j] + other.mat[i][j];
    }
    return c;
}

EDIT: Apparently this is bad practice, guess I also learned something today :)

edited Feb 21 '13 at 19:24

answered Feb 21 '13 at 14:49

MrHug

1,315
10
27

eww... gross... Please don't do this. – Benjamin Lindley Feb 21 '13 at 14:51
The question and potential problem of this approach is who will destroy the c object and how we can be sure it really do it. (Memory leaks.) – Al Kepp Feb 21 '13 at 14:53
I would say that what you want is new object for the + operator? So what's wrong with creating a new one? – MrHug Feb 21 '13 at 15:21

C++ Destructor being called in overloaded arithmetic operators

6 Answers6

Linked