There is lots of issues about this code. As the question is about memory leaks, I can start by saying that you NEVER EVER should return a reference from a function, unless it's a part of the input. There's several reasons for this, in your case it's about memory leaks, because you're not using delete on a dynamically allocated object... hence a memory leak.
Lets take it from the top and walk through the problems with the code.
The first 2 constructors are fine.
The Copy constructor does not check if other.mat is a NULL pointer, and will fail if you'd do something like this (crashes):
matrix m( matrix() )
The Destructor has the same problem as the Copy constructor - it doesn't check if
mat is a NULL pointer. So this would fail (crashes):
1 2
|
matrix * m = new matrix;
delete m;
|
The operator+ overload:
First of all - this operator should only work if the matrix sizes are equal, therefore a call like this will fail - and cause a crash - as Peter87 said:
1 2 3 4 5 6 7 8
|
matrix m1(500, 500), m2(600, 600), m3;
for (int i=0; i<600; ++i)
for (int j=0; j<600; ++j) {
if (i < 500 && j < 500)
m1.element(i, j) = 1;
m2.element(i, j) = 2;
}
m3 = m1 + m2;
|
As I said earlier - you should NEVER dynamically allocate and return a pointer/reference to something that requires a delete operation. Instead you should do what Cubbi suggested.
However this will cause a copy on assignment, unless your compiler is smart enough to remove that copy with optimizing. This is a known issue about C++98 or earlier, there is no way around it unless you code in an memory leak prone way, such as returning a pointer to a dynamically allocated object - like you did.
However! If you're using C++0x/C++11 you can make use of the rvalue references, and write an assignment operator function like this:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40
|
// Old fashion assignment operator
matrix& operator=(const matrix & other)
{
// We are forced to copy here..
cols = other.cols;
rows = other.rows;
// Remove any old data
if (!mat) {
for( int i = 0 ; i < rows; ++i )
if( mat[i] ) delete [] mat[i];
if( mat ) delete [] mat;
}
// Init mat and copy over the data from other
mat = new double* [rows];
for( int i = 0; i < rows; ++i )
{
mat[i] = new double [ cols ];
for( int j = 0; j < cols; ++j )
if (!other.mat && !other.mat[i])
mat[i][j] = other.mat[i][j];
else
mat[i][j] = 0;
}
return *this;
}
// C++0x/C++11: rvalue reference assignment operator
matrix& operator=(matrix && other)
{
cols = other.cols;
rows = other.rows;
mat = other.mat;
other.cols = other.rows = 0;
other.mat = 0;
return *this;
}
|
The code above will make your...
M3 = M1 + M2;
...statement call the rvalue reference version of the assignment operator. This call only copies
an address to your matrix, and not the matrix itself, therefore you're only copying a few bytes, 2 ints and a pointer.
You can also use rvalue references to create a copy constructor - in the exact same way as the assignment operator so that it too uses rvalue references whenever it can.
Hope it helps!
// Sorglig