memory leak in class code

[florian kno]

hello all!

i'm using gcc 3.2 on linux.
i have a (self developed) matrix class:

class cMatrix { // unneccessary info stripped
....
double *data; // matrix data is stored in a heap array

cMatrix row(const int& r) const;
// const cMatrix& row(const int& r) const; // is this better?? why?
....
const double& dotproduct(cons t cMatrix& m1, const cMatrix& m2) const;
....
}

cMatrix cMatrix::row(co nst int& r) const {
cMatrix *tmp=new cMatrix(1,ncols );
for(int i=0;i<ncols;i++ ) (*tmp)[i]=data[r*ncols + i];
return *tmp;
}

const double& cMatrix::dotpro duct(const cMatrix& v1, const cMatrix& v2)
const {
double d=0.0;
for(int i=0;i<v1.elemen ts();i++) d+=v1[i]*v2[i];
return d;
}
int main() {

{
cMatrix m(10,10);
cMatrix r=m.row(1);
}

return 0;
}

using valgrind i found out that the above main() leaks memory.

my questions are:
* how to alter the code of cMatrix::row to avoid memory leaks?
* how to avoid objects stored on the stack that get overriden later
when program code is further executed?
* is there a better syntax for (*tmp)[i]="some value"?

additional info:
* i later definitly need something like
res=dotproduct( Ni, nodes.column(i) ); (where Ni is also a matrix)
to work correctly.(^^^^ ^^^^^^^^^^^ temporary object)
-> operator and method chaining.

thanks for any help or ideas!

--
flo

[Peter van Merkerk]

"florian kno" <fl****@yahooQ. com> wrote in message
news:lV******** **************@ news.chello.at. ..

hello all!

i'm using gcc 3.2 on linux.
i have a (self developed) matrix class:

class cMatrix { // unneccessary info stripped
...
double *data; // matrix data is stored in a heap array

cMatrix row(const int& r) const;
// const cMatrix& row(const int& r) const; // is this better?? why? ...
const double& dotproduct(cons t cMatrix& m1, const cMatrix& m2) const; ...
}

cMatrix cMatrix::row(co nst int& r) const {
cMatrix *tmp=new cMatrix(1,ncols );
for(int i=0;i<ncols;i++ ) (*tmp)[i]=data[r*ncols + i];
return *tmp;
}

const double& cMatrix::dotpro duct(const cMatrix& v1, const cMatrix& v2) const {
double d=0.0;
for(int i=0;i<v1.elemen ts();i++) d+=v1[i]*v2[i];
return d;
}
int main() {

{
cMatrix m(10,10);
cMatrix r=m.row(1);
}

return 0;
}

using valgrind i found out that the above main() leaks memory.

my questions are:
* how to alter the code of cMatrix::row to avoid memory leaks?
Don't use 'new' when you don't have to:

cMatrix cMatrix::row(co nst int& r) const
{
cMatrix tmp(1,ncols);
for(int i=0;i<ncols;i++ )
{
tmp[i] = data[r*ncols + i];
}
return tmp;
}

In C++ you don't have to use 'new' to instatiate class objects.
* how to avoid objects stored on the stack that get overriden later
when program code is further executed?
I don't understand this question???
* is there a better syntax for (*tmp)[i]="some value"?
If you don't use pointers like in the (untested) code I showed you, you
don't get the ugly syntax.
additional info:
* i later definitly need something like
res=dotproduct( Ni, nodes.column(i) ); (where Ni is also a matrix)
to work correctly.(^^^^ ^^^^^^^^^^^ temporary object)
-> operator and method chaining.

I don't see your problem here.

--
Peter van Merkerk
peter.van.merke rk(at)dse.nl

[florian kno]

hello!

Peter van Merkerk wrote:

"florian kno" <fl****@yahooQ. com> wrote in message
news:lV******** **************@ news.chello.at. ..

my questions are:
* how to alter the code of cMatrix::row to avoid memory leaks?

Don't use 'new' when you don't have to:

cMatrix cMatrix::row(co nst int& r) const
{
cMatrix tmp(1,ncols);
for(int i=0;i<ncols;i++ )
{
tmp[i] = data[r*ncols + i];
}
return tmp;
}

In C++ you don't have to use 'new' to instatiate class objects.

ok.

* how to avoid objects stored on the stack that get overriden later
when program code is further executed?

I don't understand this question???

i was thinking that the local objects are stored on the stack (that's why i
created new ones not on the stack) and that a reference is returned to
memory space that is later overwritten.

additional info:
* i later definitly need something like
res=dotproduct( Ni, nodes.column(i) ); (where Ni is also a matrix)
to work correctly.(^^^^ ^^^^^^^^^^^ temporary object)
-> operator and method chaining.

I don't see your problem here.

that's somewhat redundant with my stack problem.

thanks.

--
flo

[Peter van Merkerk]

> > Don't use 'new' when you don't have to:

cMatrix cMatrix::row(co nst int& r) const
{
cMatrix tmp(1,ncols);
for(int i=0;i<ncols;i++ )
{
tmp[i] = data[r*ncols + i];
}
return tmp;
}

In C++ you don't have to use 'new' to instatiate class objects. ok.

* how to avoid objects stored on the stack that get overriden later
when program code is further executed?

I don't understand this question???

i was thinking that the local objects are stored on the stack (that's

why i created new ones not on the stack) and that a reference is returned to
memory space that is later overwritten.

It appears to me that you have a Java mindset. If that is the case keep
telling youself C++ is not Java, even though the syntax looks similar.
Java has reference semantics, C++ has value semantics. This means that
when the function cMatrix::row() returns the tmp object will be copied
into the object that receives the return value (you did implement a copy
constructor, did you?). After the tmp object has been copied, it will be
destroyed. A smart optimizer may be able to optimize the copying away,
but that should be of no concern to you.

Your concern would be valid if cMatrix::row() would return references
(cMatrix&) to local objects. An alternative for your orignal
implementation of the cMatrix::row() function would to let it return a
pointer (cMatrix*), and put the burden of deleting the object on the
caller. It avoids copying of the matrix but generally you want to avoid
this kind of construct, as it is inconvenient for the caller and it may
easilly lead to memory leaks.

--
Peter van Merkerk
peter.van.merke rk(at)dse.nl

[florian kno]

Peter van Merkerk wrote:

It appears to me that you have a Java mindset. If that is the case keep
yes, i have a java background. using your hints i have been able to reduce
my memory leaks from 80 to 3. and these 3 will be killed shortly.

the result of my program is still the same so things work.
telling youself C++ is not Java, even though the syntax looks similar.
Java has reference semantics, C++ has value semantics. This means that
when the function cMatrix::row() returns the tmp object will be copied
into the object that receives the return value (you did implement a copy
constructor, did you?).
yes, i have.
After the tmp object has been copied, it will be
destroyed. A smart optimizer may be able to optimize the copying away,
but that should be of no concern to you.

Your concern would be valid if cMatrix::row() would return references
(cMatrix&) to local objects. An alternative for your orignal
implementation of the cMatrix::row() function would to let it return a
pointer (cMatrix*), and put the burden of deleting the object on the
caller. It avoids copying of the matrix but generally you want to avoid
this kind of construct, as it is inconvenient for the caller and it may
easilly lead to memory leaks.

later, when my program continues to grow, i want to do (actually i have to):

result=(matrix1 .row(4).crosspr oduct(matrix2.c olumn(2)).trans pose()+matrixte mp
).dotproduct(ma trix3.inverse() ); // something written here

:))

thanks for your help!

--
flo

[Jesper Madsen]

Did you implement a destructor?
And did you do a assignment operator and a copy constructor???
If you pass this cMatrix around, i believe you need it ;)

"florian kno" <fl****@yahooQ. com> wrote in message
news:lV******** **************@ news.chello.at. ..

hello all!

i'm using gcc 3.2 on linux.
i have a (self developed) matrix class:

class cMatrix { // unneccessary info stripped
...
double *data; // matrix data is stored in a heap array

cMatrix row(const int& r) const;
// const cMatrix& row(const int& r) const; // is this better?? why?
...
const double& dotproduct(cons t cMatrix& m1, const cMatrix& m2) const; ...
}

cMatrix cMatrix::row(co nst int& r) const {
cMatrix *tmp=new cMatrix(1,ncols );
for(int i=0;i<ncols;i++ ) (*tmp)[i]=data[r*ncols + i];
return *tmp;
}

const double& cMatrix::dotpro duct(const cMatrix& v1, const cMatrix& v2)
const {
double d=0.0;
for(int i=0;i<v1.elemen ts();i++) d+=v1[i]*v2[i];
return d;
}
int main() {

{
cMatrix m(10,10);
cMatrix r=m.row(1);
}

return 0;
}

using valgrind i found out that the above main() leaks memory.

my questions are:
* how to alter the code of cMatrix::row to avoid memory leaks?
* how to avoid objects stored on the stack that get overriden later
when program code is further executed?
* is there a better syntax for (*tmp)[i]="some value"?

additional info:
* i later definitly need something like
res=dotproduct( Ni, nodes.column(i) ); (where Ni is also a matrix)
to work correctly.(^^^^ ^^^^^^^^^^^ temporary object)
-> operator and method chaining.

thanks for any help or ideas!

--
flo

[florian kno]

Jesper Madsen wrote:

Did you implement a destructor?
And did you do a assignment operator and a copy constructor???
If you pass this cMatrix around, i believe you need it ;)

actually i have all of the above mentioned methods.
problem was solved by skipping the pointer stuff (*).
"florian kno" <fl****@yahooQ. com> wrote in message
news:lV******** **************@ news.chello.at. ..

cMatrix cMatrix::row(co nst int& r) const {
cMatrix *tmp=new cMatrix(1,ncols );
for(int i=0;i<ncols;i++ ) (*tmp)[i]=data[r*ncols + i];
return *tmp;
}

is now:

const cMatrix cMatrix::row(co nst int& r) const {
cMatrix tmp(1,ncols);
for(int i=0;i<ncols;i++ ) tmp[i]=data[r*ncols + i];
return tmp;
}

--
flo

[Peter van Merkerk]

> > "florian kno" <fl****@yahooQ. com> wrote in message

news:lV******** **************@ news.chello.at. ..

cMatrix cMatrix::row(co nst int& r) const {
cMatrix *tmp=new cMatrix(1,ncols );
for(int i=0;i<ncols;i++ ) (*tmp)[i]=data[r*ncols + i];
return *tmp;
}

is now:

const cMatrix cMatrix::row(co nst int& r) const {
cMatrix tmp(1,ncols);
for(int i=0;i<ncols;i++ ) tmp[i]=data[r*ncols + i];
return tmp;
}

If you return by value like in this case, there is no point in putting const
before cMatrix. It doesn't do any harm, but it doesn't do any good either.
Since the caller gets a copy of the matrix generated by cMatrix::row(), it
doesn't matter what the caller does with the copy as it won't affect the
object that produced the copy.

If you would return by reference (not a viable option in this example), or
pass values by reference you should use const whenever possible. For
primitive types like 'int' there is usually no benefit in passing them by
const reference because copying is extremely fast for these types. Passing
them by value is often just as fast (if not faster), and results in clearer
code. For objects whose copy constructor may take some time (your cMatrix
class is an example of this) it is advisable to pass them by const
references.

--
Peter van Merkerk
peter.van.merke rk(at)dse.nl

[Big Brian]

> > Did you implement a destructor?

And did you do a assignment operator and a copy constructor???
If you pass this cMatrix around, i believe you need it ;)

actually i have all of the above mentioned methods.
problem was solved by skipping the pointer stuff (*).

cMatrix cMatrix::row(co nst int& r) const {
cMatrix *tmp=new cMatrix(1,ncols );
for(int i=0;i<ncols;i++ ) (*tmp)[i]=data[r*ncols + i];
return *tmp;
}

is now:

const cMatrix cMatrix::row(co nst int& r) const {
cMatrix tmp(1,ncols);
for(int i=0;i<ncols;i++ ) tmp[i]=data[r*ncols + i];
return tmp;
}

This solves the memory leak, however you might not want to do this if
your matrix is big since it creates a temporary object, and copies the
data twice. You may not want to do for perfance and resource issues,
if your matrix is large. Oh I just re-read the original code, and
since it was returning *tmp, it has the same issue.

I have no idea how big your matrices are going to be, but if they're
going to be large, you may want to think about another solution.
Since your row function returns a const cMatrix, you could (maybe
somehow) return a pointer into "data", and thus not have to copy it,
which you're doing twice in your current code.

const cMatrix cMatrix::row(co nst int& r) const {
cMatrix tmp(1,ncols);
for(int i=0;i<ncols;i++ ){
//COPYING THE DATA HERE...
tmp[i]=data[r*ncols + i];
}
return tmp;
}

and when this is used...

const cMatrix m = (someobject).ro w(x); // copying the temp object
here, ie copying the data a second time

Maybe it's not an issue, but maybe it is too.

-Brian

[Peter van Merkerk]

> > is now:

const cMatrix cMatrix::row(co nst int& r) const {
cMatrix tmp(1,ncols);
for(int i=0;i<ncols;i++ ) tmp[i]=data[r*ncols + i];
return tmp;
}
This solves the memory leak, however you might not want to do this if
your matrix is big since it creates a temporary object, and copies the
data twice. You may not want to do for perfance and resource issues,
if your matrix is large. Oh I just re-read the original code, and
since it was returning *tmp, it has the same issue.

The best approach is to make right first, and make it fast later.
I have no idea how big your matrices are going to be, but if they're
going to be large, you may want to think about another solution.
Since your row function returns a const cMatrix, you could (maybe
somehow) return a pointer into "data", and thus not have to copy it,
which you're doing twice in your current code.
A good optimizer (one that supports return value optimization) might be able
to optimize the redundant copying away. If that is the case returning
pointers would be no faster. The problem with returning pointers is that it
is very inconvient for the caller. For example try transforming this to a
version with pointers that doesn't leak:
result=(matrix1 .row(4).crosspr oduct(matrix2.c olumn(2)).trans pose()+matrixte m
p).dotproduct(m atrix3.inverse( ));

Yes, it is possible to do that. But it also terribly inconvenient; it is
extremely easy for the caller to leak memory, it would take many more lines
than the one without pointers and the resulting code would be a lot less
readable.
const cMatrix cMatrix::row(co nst int& r) const {
cMatrix tmp(1,ncols);
for(int i=0;i<ncols;i++ ){
file://COPYING THE DATA HERE...
tmp[i]=data[r*ncols + i];
}
return tmp;
}

and when this is used...

const cMatrix m = (someobject).ro w(x); // copying the temp object
here, ie copying the data a second time

Maybe it's not an issue, but maybe it is too.

If it is an issue (and it may very well be so when dealing with large
matrixes and less than perfect optimizers), an alternative could be to use a
proxy object. This technique has been used often used with string
implementations . The advantage of this technique is that it transparent for
the caller and that it puts the burden on the implementor(s) of the cMatrix
class rather than its users. But this technique has downsides too and it is
easy to get it wrong leading to very obscure bugs.

But like I said before first get it right. If it is too slow, use a profiler
to pinpoint the performance bottlenecks. If it is the copy constructor then
you may consider the techniques discussed here. OTOH the performance
bottleneck might be just as well somewhere else (matrix computations tend to
burn plenty of CPU cycles as well). Anyway I assume this is just an
excersise for the OP. There are plenty of C++ Matrix libraries around, so if
it is a real project it would be more logical to look at those first.

--
Peter van Merkerk
peter.van.merke rk(at)dse.nl