push a 2D array into a vector and delete the array, but cause segmentation fault

Question

I have a class named Matrix. There is a 2D array in the class to save the data.

    template <class Type>
    class Matrix{
        public:
           Matrix(int row, int col){
             rows = row; cols = col;
             data = new Type*[rows];
             for(int i = 0; i < rows; i++){
              data[i] = new Type[cols]; 
              }
           }
      public: 
         int rows;
         int cols;
         Type **data;
    };

And I have a vector to save the Matrix. Every time I have a Matrix, I will push back the matrix in this vector for future calculation. In order to avoid memory leak, I want to delete the Matrix after pushing it back to the vector. But if I don't delete it, the program works; if I delete it (as shown in the code below), the program will show the segmentation fault when I want to do some calculation for this vector. The code I used is shown below

    vector<Matrix<int>> v; 
    for(int i = 0; i < 10; ++i){
         Matrix<int> a(3,3);
                 ...... // I fill the elements of a.data
       v.push_back(a);
       for(int j = 0; j < a.rows; ++j)
             delete[] a.data[j];
       delete[] a.data;
    }

Hope I have explained my problem clearly. If anything makes you confuse, please comment me.

Thanks for help!!!

Answer 1

I see multiple problems in your code:

• it's C++ and you are manually allocating memory for the matrix, why?
• even if you have access to destructors you don't implement it but you delete data for the matrix manually in the main code
• your approach doesn't manage memory clearly, when you push_back by value, the Matrix is copied inside the vector, who owns the pointers for the data at this point? The copy on stack or the copy inside the vector?

You should take care of carefully manage memory by implementing a correct copy constructor, copy assignment operator and destructor for the class.

But this is irrelevant since you can just use C++ features and forget about these problems, some solutions:

Store pointers in the vector

class Matrix{
public:
  Matrix(int row, int col){
    rows = row; cols = col;
    data = new Type*[rows];
    for(int i = 0; i < rows; i++){
      data[i] = new Type[cols];
    }
  }
  ~Matrix() {  // you need a destructor
    for (int i = 0; i < rows; ++i)
      delete[] data[i];
    delete data;
  }
public:
  int rows;
  int cols;
  Type **data;
};

std::vector<std::unique_ptr<Matrix>> v;
Matrix* matrix = new Matrix(3,3);
v.push_back(std::unique_ptr<Matrix>(matrix));

Now matrices become persistent but they'll get automatically released when v goes out of scope (thanks to unique_ptr and destructor)

Use a std::vector / std::array for matrix elements

You are using them to store multiple matrices, why don't you use them also for matrix itself?

template<size_t ROWS, size_t COLS, class TYPE>
class Matrix
{
  std::array<std::array<COLS, TYPE>, ROWS> data;
  ...
}

Now everything is automatically managed, you don't need to release memory and you don't need a destructor for Matrix at all.

std::vector<Matrix<3,3,float>> v;
Matrix<3,3,float> m;
v.emplace_back(m);
m.data[0][0] = 1;

If you want to have different sizes of matrices in same vector or if you want to keep stack usage low (since std::array is not dynamically allocated) then use a std::vector instead that an std::array so that you can remove the template arguments.

Answer 2

Your Matrix class doesn't have a proper copy constructor so when you push the new matrix to the vector all the fields are copied to a new created Matrix inside the vector.

AND Matrix::data is also copied as a pointer. Which means that the new Matrix inside the vector points to the same Matrix a that you created in the for loop. Thus, when you delete a you actually make the Matrix inside the vector invalid.

Answer 3

OP's problem is a classic Rule of Three violation.

The class uses raw pointers to memory allocated by the constructor

Matrix(int row, int col)
{
    rows = row; cols = col;
    data = new Type*[rows];
    for(int i = 0; i < rows; i++)
    {
        data[i] = new Type[cols]; 
    }
}

The destructor deletes all of the memory so there will be no leak.

~Matrix() 
{  // you need a destructor
    for (int i = 0; i < rows; ++i)
        delete[] data[i];
    delete data;
}

However, there are no copy or move constructors or assign or move operators, so the defaults will simply copy the pointers, resulting in two objects pointing to the same memory.

Not only are both copies modified when one is changed, but when one is deleted, the other copy's pointers are rendered invalid.

This is generally seen as bad.

Solution one is create copy and move constructors and assignment and move operators, but that is a bit of work to get right.

Thankfully, std::vector gets it right out of the box.

template<class Type>
class Matrix{
public:
  Matrix(int row, int col):rows(row), cols(col), data(rows, std::vector(cols))
  {
    // does nothing. All of the heavy lifting was in the initializer
  }
  // no longer need a destructor.
public:
  int rows;
  int cols;
  std::vector<std::vector<type>> data;
};

The next bit I suggest as a performance enhancement. Because the vector of vectors is literally a vector containing other vectors, it's not all in one memory block. Having to jump around in RAM to find the next bit of data can be costly. Look up Cache Miss and Spatial locality to see why.

template<class Type>
class Matrix{
public:
  Matrix(int row, int col):rows(row), cols(col), data(rows*cols)
  {
    // does nothing. All of the heavy lifting was in the initializer
  }
  // no longer need a destructor.
  //add a convenience method for easy access to the vector
  type & operator()(size_t row, size_t col)
  {
    return data[row*cols+col];
  } 
  type operator()(size_t row, size_t col) const
  {
    return data[row*cols+col];
  } 
private: // note change of access to private Best to keep ones data to one's self
  int rows;
  int cols;
  std::vector<type> data;
};

Now your're safe for stuff like:

std::vector<Matrix<float>> v;
Matrix<float> m(3,3);
v.emplace_back(m);
m(0,0) = 1;