Copy constructor of dynamic two-dimensional array

I need help. Now i'm trying to make a class Matrix, but my program freezes every time I run it in Visual Studio 2013. I think there is some trouble with copy constructor. Here is the whole code. `

class Matrix { private: int** matrix; int X; // Matrix rows int Y; // Matrix columns public: // Default Constructor Matrix() { X = 0; Y = 0; matrix = NULL; } // Constructor with parameters Matrix(int _X, int _Y) { srand(time(NULL)); X = _X; Y = _Y; matrix = new int*[X]; for (int i = 0; i < X; i++) matrix[i] = new int[Y]; for (int i = 0; i < X; i++) { for (int j = 0; j < Y; j++) { matrix[i][j] = rand() % 100; } } } // Copy constructor Matrix(const Matrix& N) { X = N.X; Y = N.Y; matrix = new int*[X]; for (int i = 0; i < X; i++) matrix[i] = new int[Y]; for (int i = 0; i < X; i++) { for (int j = 0; j < Y; j++) { matrix[i][j] = N.matrix[i][j]; } } } // Destructor ~Matrix() { for (int i = 0; i < X; i++) delete[] matrix[X]; } //-------------------------------------- void ShowMatrixOnScreen() { for (int i = 0; i < X; i++) { for (int j = 0; j < Y; j++) cout << matrix[i][j] << " "; cout << endl << endl; } } }; void main() { Matrix x(4, 2); x.ShowMatrixOnScreen(); } 

`

Function "ShowMatrixOnScreen" prints matrix "x" on a screen, but then console freezes.

2 Answers

Your destructor has the statement delete[] matrix[X];

matrix[X] does not exist, so you are freeing memory which is not allocated to you, which is Undefined Behavior. It should be delete [] matrix[i] instead!

Moreover, as pointed by Vlad From Moscow, it is advisable to delete all the memory, so you shall also consider adding delete [] matrix, to delete the 1-D array of pointers which you allocated using matrix = new int*[X];

The problem is that your destructor is invalid

First of all in this loop

 for (int i = 0; i < X; i++) delete[] matrix[X]; 

it tries to access non-existent element matrix[X] beyond the allocated array. The valid range for indices is [0, X - 1]

So you have to write

 for (int i = 0; i < X; i++) delete[] matrix[i]; ^^^ 

But thsi loop is valid only if matrix is not equal to nullptr Therefore before the loop you have to check whether matrix is equal to nullptr or not equal.

And also you need to delete the memory allocated to the first one-dimensionl array

delete [] matrix; 

Thus the destructor will look like

~Matrix() { if ( matrix ) { for ( int i = 0; i < X; i++ ) delete []matrix[i]; } delete []matrix; } 

Also in the copy constructor you should also check whether data member matrix of the copied object also is not equal to nullptr

// Copy constructor Matrix(const Matrix& N) { X = N.X; Y = N.Y; if ( N.matrix ) { matrix = new int*[X]; for (int i = 0; i < X; i++) matrix[i] = new int[Y]; for (int i = 0; i < X; i++) { for (int j = 0; j < Y; j++) { matrix[i][j] = N.matrix[i][j]; } } } } 

It would be much better if data members X and Y has type size_t. Otherwise you need also to check whether they are not negative.

The allocation of the memory could be placed in a separate private member function.

for example

int ** allocate( size_t m, size_t n ) { int **p = nullptr; if ( n != 0 && m != 0 ) { matrix = new int *[m]; for ( size_t i = 0; i < m; i++ ) matrix[i] = new int[n]; } return p; } 

In the class definition data members X and Y should precede data member matrix

private: int X; // Matrix rows int Y; // Matrix columns int** matrix; 

In this case you could use mem-initializer list of constructors to initialize the class data members.

For example

// Constructor with parameters Matrix(int X, int Y) : X( X ), Y( Y ), matrix( allocate( X, Y ) ) { if ( matrix ) { srand(time(NULL)); for (int i = 0; i < X; i++) { for (int j = 0; j < Y; j++) { matrix[i][j] = rand() % 100; } } } } 

And the default constructor could be defined like

// Default Constructor Matrix() : Matrix( 0, 0 ) { } 

In turn the copy constructor could be defined like

// Copy constructor Matrix( const Matrix &N ) : X( N.X ), Y( N.Y ), matrix( allocate( N.X, N.Y ) ) { if ( matrix ) { for (int i = 0; i < X; i++) { for (int j = 0; j < Y; j++) { matrix[i][j] = N.matrix[i][j]; } } } } 

And you should define also the copy assignment operator or define it as deleted. Otherwise there will be problems when you try to assign one object of the class to another.

Take into account that function main shall have return type int

int main()