C++ operator destroys input variables

Question

This is probably just me being stupid somehow or the other, but I am relatively new to C++, so forgive me the idiocy. I'm trying to teach myself how to use operators. I've defined a very basic operator as follows:

Matrix::Matrix operator+(Matrix a1, Matrix a2) {
    if(a1.rows != a2.rows || a1.cols != a2.cols) {
        std::cout << "ERROR: Attempting to add matrices of non-equal size." << std::endl;
        exit(6);
    }

    int i, j;

    Matrix c(a1.rows, a1.cols);

    for(i = 0; i < a1.rows; i++)
        for(j = 0; j < a1.cols; j++)
            c.val[i][j] = a1.val[i][j] + a2.val[i][j];

    return c;
}

The class Matrix represents a matrix, and has a constructor that takes two ints as input (the number of rows and columns in the matrix, respectively), and creates a 2D array of doubles of the appropriate size (named val). This function works as supposed to in that the value for c is correct, but it also appears to destruct a1 and a2. That is, if I write

Matrix c = a + b;

It gives the right result, but a and b are no longer usable, and I get a glibc error at the end of the code claiming I am trying to destruct a and b when they have already been destructed. Why is this?

Answer 1

Your signature is wrong:

Matrix operator+(Matrix a1, Matrix a2)

it should be

Matrix operator+(const Matrix& a1, const Matrix& a2)

The reason it appears to destroy a1 and a2 is because, well, it is, since those are temporary copies created in the method scope.

If the original values are destroyed, you're probably violating the rule of three. When a1 and a2 are destroyed, the destructor gets called, and you're probably deleting pointers in the destructor. But since the default copy constructor does only a shallow copy, the copied a2 and a1 will delete the original memory.

Edit: Since there are split opinions about this, I'll extend my answer:

Assume:

struct A
{
   int* x;
   A() { x = new int; *x = 1; }
   ~A() { delete x; }
};

//option 1:
A operator + (A a1, A a2)
{
   A a; return a; //whatever, we don't care about the return value
}

//option 2:
A operator + (const A& a1, const A& a2)
{
   A a; return a; //again, we don't really care about the return value
}

In this first example, the copy constructor is not implemented.

A copy constructor is generated by the compiler. This copy constructor copies x into the new instance. So if you have:

A a;
A b = a;
assert( a.x == b.x );

Important note that the pointers are the same.

Calling option 1 will create copies inside operator +, because the values are passed by value:

A a;
A b;
a + b; 
//will call:
A operator + (A a1, A a2)
// a1.x == a.x
// a2.x == n.x

When operator + exits, it will call delete x on objects a1 and a2, which will delete the memory that is also pointed to by a.x and b.x. That is why you get the memory corruption.

Calling option 2 however, since no new objects are created because you pass by reference, the memory will not be deleted upon function return.

However, this isn't the cleanest way to solve the issue. It solves this issue, but the underlying one is much more important, as Konrad Pointed out, and I have in my original answer (although haven't given it enough importance, I admit).

Now, the correct way of solving this is properly following the rule of three. That is, have an implementation for destructor, copy constructor and assignment operator:

struct A
{
   int* x;
   A() { x = new int; *x = 1; }
   A(const A& other)  //copy constructor
   {
      x = new int;  // this.x now points to a different memory location than other.x
      *x = other.(*x);  //copy the value though
   }
   A& operator = (const A& other) //assignment operator
   { 
      delete x; //clear the previous memory
      x = new int;     
      *x = other.(*x);  //same as before
   }
   ~A() { delete x; }
};

With this new code, let's re-run the problematic option 1 from before:

A a;
A b;
a + b; 
//will call:
A operator + (A a1, A a2)
// a1.x != a.x
// a2.x != n.x

Because the copies a1 and a2 now point to different memory locations, when they are destroyed, the original memory is intact and the original objects - a and b remain valid.

Phew! Hope this clears things up.

Answer 2

I’m assuming that you are allocating dynamic memory inside the Matrix class using new and didn’t implement a custom copy constructor, thus violating the rule of three.

The cause of the error would then be that the local copy of the Matrix instances reuses the dynamic memory of the argument instances, and their destructors free it at the end of the method.

The solution is very simple: Don’t use pointers. Instead, you could use a nested std::vector to store the data.

Furthermore, the head of the operator is mangled. Depending on where you declare the function, it must either look like this, if you define the operator inside the class:

ReturnType operator +(Arg1)

Or, if you define it outside the class, it needs to look like this:

ReturnType operator +(Arg1, Arg2)

Yours is a wild mix of both. I’m assuming that your operator definition should look as follows:

Matrix operator +(Matrix a, Matrix b) { … }

If your code compiles then this is either an error in the compiler or you are using a very weird class structure indeed. Furthermore, as Luchian pointed out, it’s more efficient to pass the arguments as const& rather than by value. However, you should first make your code run correctly.