program works as excpected untill I add a destructor to it

Question

#include <iostream>
using namespace std;
class Matrix {

public:
  long int **Matr;
  long int n;

  void Create() {
    Matr = new long int *[n];
    for (int z = 0; z < n; z++)
      Matr[z] = new long int[n];
  }
  // constructors and destructor
  Matrix() : n(5) { Create(); }

  Matrix(long int i) : n(i) { Create(); }

  // Copy constructor
  Matrix(Matrix &N) {
    n = N.n;

    Matr = new long int *[n];

    for (int i = 0; i < n; i++)
      Matr[i] = new long int[n];

    for (int i = 0; i < n; i++) {
      for (int j = 0; j < n; j++) {
        Matr[i][j] = N.Matr[i][j];
      }
    }
  }

  Matrix operator*(Matrix &mx) {
    int i, j, k, num;
    Matrix result(n);
    for (int i = 0; i < n; i++)
      for (int j = 0; j < n; j++)
        num = 0;
    for (int k = 0; k < n; k++) {
      num += Matr[i][k] * mx.Matr[k][j];
    }
    result.Matr[i][j] = num;
    return result;
  }
  ~Matrix() {
    for (int z = 0; z < n; z++) {
      delete[] Matr[z];
    }
    delete[] Matr;
  }

  void Display() {
    for (int i = 0; i < n; i++) {
      for (int j = 0; j < n; j++) {
        cout << Matr[i][j];
      }
      cout << endl;
    }
  }

  Matrix operator+(Matrix &mx) {
    Matrix result(n);
    for (int i = 0; i < n; i++) {
      for (int j = 0; j < n; j++) {
        result.Matr[i][j] = Matr[i][j] + mx.Matr[i][j];
        // cout << result.Matr[i][j] << "\n";
      }
    }
    return result;
  }
};

int main() {
  Matrix M(2);
  M.Matr[0][0] = 0;
  M.Matr[0][1] = 1;
  M.Matr[1][0] = 2;
  M.Matr[1][1] = 3;

  Matrix N(2);
  N.Matr[0][0] = 0;
  N.Matr[0][1] = 1;
  N.Matr[1][0] = 2;
  N.Matr[1][1] = 3;

  Matrix C;

  C = M + N;
  cout << C.Matr[0][0];

  return 0;
}

I am doing a Matrix OOP class with some basic methods. I can't do "+" operator work in the way I want. In this case, when I'm trying to print C.Matr[0][0] it's printing random number for some reason, but if I simply remove destructor from code it works as expected and printing 0. Can someone help me to figure out why this is happening? Thanks a lot.

Answer 1

C is assigned a temporary with a shared Matr. The temporary dies immediately and you delete dangling pointers in the dtor.

The rule of three/five/zero will help you to prevent it.

---

What you are missing a copy assignment operator:

Matrix& operator=(const Matrix& N)
{
    // copy to temporary matrix
    long int tmpN = N.n;
    long int** tmpMatr = new long int*[tmpN];
    for (int i = 0; i < tmpN; i++)
        tmpMatr[i] = new long int[tmpN];

    for (int i = 0; i < tmpN; i++)
    {
        for (int j = 0; j < tmpN; j++)
             tmpMatr[i][j] = N.Matr[i][j];
    }

    // swap
    long int** swapMatr = Matr;
    long int swapN = n;
    Matr = tmpMatr;
    n = N.n;

    // deallocate old Matr
    for (int z = 0; z<swapN; z++){
        delete[] swapMatr[z];
    }  
    delete[] swapMatr;

return *this;
}

Online gdb

Answer 2

You must implement the assignment operator, not just the copy constructor and destructor.

The simplest way to implement the assignment operator is to use the copy / swap idiom:

  #include <algorithm>
  //...
  class Matrix
  {
     //...
     int **Matr;
     int n;
     //...
     public:
        Matrix& operator=(const Matrix& m)
        {
           if ( this != &m )
           {
              Matrix temp(m);  // create a copy of the passed in value
              std::swap(temp.Matr, Matr);  // swap out the internals with *this
              std::swap(temp.n, n);
           }  // Here temp's destructor removes the old memory that *this 
              // previously had
           return *this;
        }
        //...
     };

Note that this only works if you have a working, non-buggy copy constructor and destructor.

In addition, your copy constructor should be taking a const reference, not just a reference to Matrix:

Matrix(const Matrix& m) { ... }

Once you make those changes, the program no longer crashes, as seen here.

Answer 3

You don't have an assignment operator, so who knows what the line C = M + N; is doing?

Well, it's using the default assignment operator, which just copies each member in turn. In particular, the pointer Matr is copied from the pointer in the temporary M+N, so it points to memory which was allocated for the temporary, which is then destroyed.

Before you added the destructor, you had a memory leak, so the values persisted in the heap, and it appeared to work. After you added a destructor which freed the memory, it was apparently quickly overwritten.