Sign in to follow this  
hannes100

Memory leaks

Recommended Posts

Hi, i wrote some math and physiks library to implement some realistic opengl graphics. At the moment it works fine with some balls(velocity force torque ......). But if i turn on destructor in my Vector class, i get segmentation faults. But if i turn it of, i get memory leaks, so big, that after 5 minutes the programm takes 50 % of the memory ..... so myby somebody can help me, here is some code example: class Vektor { public: Vektor(int); Vektor(int, int); ~Vektor(); Vektor operator = (Vektor); Vektor operator + (Vektor); Vektor operator * (Vektor); Vektor operator - (Vektor); Vektor operator * (double); static double euclideanScalarProduct(Vektor*, Vektor*); double euklidnorm(); double unorm(); void setvalue(int, double); void setvalue(double*); double* getvalue(); double getvalue(int); int getdim(); double* a; int dimension; }; #include "Vektor.h" #include <stdio.h> #include <math.h> Vektor::Vektor(int dim) { a=new double [dim]; dimension = dim; for (int i =0;i<dim;i++) { a[i]=0; } } Vektor::Vektor(int dim, int k) { a=new double [dim]; for (int i =0;i<dim;i++) { a[i]=0; } a[k]=1; dimension=dim; } Vektor::~Vektor() { if(a != NULL) { //delete[] a; //if i turn on, i get segfault... } } Vektor Vektor::operator = (Vektor v) { dimension=v.dimension; if(a == NULL) { a = new double[dimension]; } for (int i=0; i<v.getdim();i++) { a[i]=v.a[i]; } return(*this); } Vektor Vektor::operator + (Vektor v) { Vektor erg(v.dimension); erg.dimension=dimension; for (int i=0; i<v.dimension;i++) { erg.a[i]=a[i]+v.a[i]; } return(erg); } Vektor Vektor::operator * (Vektor v) { if(dimension != 3) { return (*this); } else { Vektor erg(dimension); erg.dimension=dimension; erg.a[0] = a[1]*v.a[2] - a[2]*v.a[1]; erg.a[1] = a[2]*v.a[0] - a[0]*v.a[2]; erg.a[2] = a[0]*v.a[1] - a[1]*v.a[0]; return(erg); } } Vektor Vektor::operator - (Vektor v) { Vektor erg(dimension); erg.dimension=dimension; for (int i=0; i<dimension;i++) { erg.a[i]=a[i]-v.a[i]; } return(erg); } Vektor Vektor::operator *(double s) { Vektor erg(dimension); erg.dimension=dimension; for (int i=0; i<dimension;i++) { erg.a[i]=s*a[i]; } return(erg); } double Vektor::euklidnorm() { double sp=0; for (int i=0; i<dimension ;i++) { sp=sp+a[i]*a[i]; } sp = sqrt(sp); return(sp); } double Vektor::unorm() { double erg=0; for (int i=0; i<dimension ;i++) { if (fabs(erg)<=fabs(a[i])) { erg=fabs(a[i]); } else {} } return (erg); } void Vektor::setvalue(int pl, double we) { a[pl]=we; } void Vektor::setvalue(double* v) { a=v; } double* Vektor::getvalue () { return(a); } double Vektor::getvalue(int pl) { return(a[pl]); } int Vektor::getdim() { return(dimension); } double Vektor::euclideanScalarProduct(Vektor *v, Vektor *w) { double scalar = 0; for (int i=0; i<v->dimension ;i++) { scalar += v->a[i] * w->a[i]; } return scalar; } thnx

Share this post


Link to post
Share on other sites


It is VERY bad to deallocate memory in the destructor with an object like a vector. Why are you using allocated memory in the first place? I'd recommend using stack memory and just creating vec2, vec3, and vec4 classes. If you really need more flexibility, you can allocate memory, but you have to be really careful.

Why? Because many people pass objects like vectors by value. You should try to pass it by reference whenever possible, but you can't guarantee that. So, what happens when you pass it by value? Well, I felt like drawing just now for some reason, so...



Basically you are calling delete[] twice on the same block of memory because when you pass by value, it creates a copy of the object which inappropriately nukes the memory.

Hope that's clear, I'm in a weird mood. :p

Share this post


Link to post
Share on other sites
you need to implement Vektor(const Vektor&) i.e. the copy ctor to have pass by value work correctly else you'll wind up double deleting, that's (probably)what's causing the segfault.
something like:

Vektor::Vektor(const Vektor &other):
{
a = new double[other.dimension];
dimension = other.dimension;
for(int i = 0; i != dimension; ++i)
a[i] = other.a[i];
}


also you might want to get into the habit of passing by const reference instead of (like you do now) passing by value since you'll get rid of a lot of copying overhead that way.

Share this post


Link to post
Share on other sites
Following DD's post, you need to make the other operators use const Vektor references too, or just Vektor references for those that modify the parameter.

You really REALLY should not be using dynamic memory for a vector, that's massive overkill.

Try using a well written class like this

Share this post


Link to post
Share on other sites
Quote:
Original post by DigitalDelusion
you need to implement Vektor(const Vektor&) i.e. the copy ctor to have pass by value work correctly else you'll wind up double deleting, that's (probably)what's causing the segfault.
something like:

Vektor::Vektor(const Vektor &other):
{
a = new double[other.dimension];
dimension = other.dimension;
for(int i = 0; i != dimension; ++i)
a[i] = other.a[i];
}


also you might want to get into the habit of passing by const reference instead of (like you do now) passing by value since you'll get rid of a lot of copying overhead that way.


And having made this work correctly, consider implementing the assignment operator in terms of the copy ctor, by the copy-and-swap idiom:


Vektor& Vektor::operator=(const Vektor& rhs) {
Vektor other(rhs); // if this throws, the object gets destroyed and
// releases its memory, and the rest is skipped by the exception.
std::swap(a, other.a); // swap pointers; now we own the "new" array, and
// "other" owns our old data
dimension = other.dimension;
// At the end of the method, the "other" Vektor falls out of scope,
// triggering its dtor, and thus cleaning up its array - which is the old
// array from the current object, due to the swap.
}


At which point it becomes possible (and really the only correct option) to use the old dtor, "delete[] a". BTW, the null checks are not necessary with C++ operators delete and delete[]; calling them on a null pointer is a no-op. (Calling them on an *invalid, non-null* pointer is undefined, but there's no portable way to detect an invalid pointer *anyway*.)

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this