Why not array deallocate when using assignment operator?-CodePudding

When running the code I keep getting error "free(): double free detected in tcache 2". How can I not deallocate new instance array? Shouldn't it be deallocate at the moment I run the second instance? Why deallocate constructor doesn't work for the n instance? How can I solve this? Should I deallocate the arr of n manually in assignment operator?

#include <iostream>
#include <iomanip>
using namespace std;
class spMatrix{
private:
    int N;
public:
    double** arr;

    spMatrix(): N(0), arr(NULL){};
    spMatrix(int size): N(size){
      N = size;
      arr = new double* [size];
      for(int i = 0; i < size; i  ) arr[i] = new double [size];
    };

    //copy constructor
    spMatrix(const spMatrix& original){
      N = original.N;
      arr = new double* [N];
      for (int i = 0; i < N;   i)
      {
        arr[i] = new double [N];
        for (int j = 0; j < N;   j)
        {
          arr[i][j] = original.arr[i][j];
        }
      }
    }


    ~spMatrix(){
       if(arr != NULL){
         for (int i = 0; i < N; i  )
         {
            if (arr[i] != NULL) delete [] arr[i];
         }
         delete [] arr;
       }
    }

    spMatrix& operator = (spMatrix const & other){
      N = other.N;
      arr = other.arr;
      for(int i = 0; i < N; i  ) {
         arr[i] = other.arr[i];
         for (int j = 0; j < N; j  ){
            arr[i][j] = other.arr[i][j];
         }
      }
      return *this;
    }

    void show(){
     for (int i = 0; i < N;   i)
     {
         for (int j = 0; j < N;   j)
         {
             cout << arr[i][j] << " ";
         }
         cout << endl;
      }
    }
};

int main(int argc, char const *argv[])
{
    spMatrix m(2);
    spMatrix n(2);
    m = n;
    m.show();

    return 0;
}

CodePudding user response：

In your copy constructor:

  N = other.N;
  arr = other.arr;
  for(int i = 0; i < N; i  ) {
     arr[i] = other.arr[i];
     for (int j = 0; j < N   1; j  ){
        arr[i][j] = other.arr[i][j];
     }
  }
  return *this;
}

This only copies/duplicates the 2nd dimension of the 2D-matrix. The first dimension is not duplicated. Try to focus your attention on just the following line:

  arr = other.arr;

If you think about this for a moment, it should be clear that you end up with two different objects having the same identical arr class member pointer. This is not correct. Different matrix objects should always be completely independent of each other and not share any pointers in common (at least not without reference-counting, and other complicated semantics).

You simply need to make a copy of arr, too. Everything else remains the same. It should probably be something like:

 arr = new double* [N];

Looks like everything else can remain the same, note how this is identical to the same allocation in the regular constructor.

However the assignment operator has the same defect, but after understanding the problem in the copy constructor it should be clear how to fix it (same fix).

Your assignment operator also has a separate bug: it leaks memory. It needs to delete the entire 2D matrix in this, before cloning it from other.

CodePudding user response：

Your problem is in the copy assignment operator:

spMatrix& operator = (spMatrix const & other){
  N = other.N;        // <<<A>>>
  arr = other.arr;    // <<<B>>>
  for(int i = 0; i < N; i  ) {
     arr[i] = other.arr[i];
     for (int j = 0; j < N   1; j  ){
        arr[i][j] = other.arr[i][j];
     }
  }
  return *this;
}

There are two significant and separate problems here. First, the current object owns some memory, an array of N arrays of length N.

However, after line <<>> you no longer know how many arrays you hold. Second, after line <<>> you no longer know the address of your arrays, and so they are leaked.

Worse, line <<>> is also now pointing this object's array into other's memory, and both objects think they own that array. When one spMatrix destructor runs, the other will still point to it and use deleted memory. When the second one is destroyed, it'll be the second delete on the already deleted memory.

All of the copying you're trying to do in those for loops is not really doing anything, since you're assigning from and to the same address.

You must allocate new buffers before copying them, and not look at the address the other object has... because it's their memory not yours to take. And you should keep the old pointers around, so that you can delete them after the allocations complete. That way if something fails you can still put it back, without messing your object up first. For example, the logic of line <<>> should be last, not first, only after everything succeeds.

CodePudding user response：

Thanks a lot.

I changed the assignment operator as below and it solved it.

spMatrix& operator = (spMatrix const & other){
   N = other.N;
   arr = new double* [N];
   for(int i = 0; i < N; i  ) {
      arr[i] = new double[N];
      for (int j = 0; j < N; j  ){
         arr[i][j] = other.arr[i][j];
      }
   }
   return *this;
 }