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=0; } } Vektor::Vektor(int dim, int k) { a=new double [dim]; for (int i =0;i<dim;i++) { a=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=v.a; } return(*this); } Vektor Vektor::operator + (Vektor v) { Vektor erg(v.dimension); erg.dimension=dimension; for (int i=0; i<v.dimension;i++) { erg.a=a+v.a; } 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=a-v.a; } return(erg); } Vektor Vektor::operator *(double s) { Vektor erg(dimension); erg.dimension=dimension; for (int i=0; i<dimension;i++) { erg.a=s*a; } return(erg); } double Vektor::euklidnorm() { double sp=0; for (int i=0; i<dimension ;i++) { sp=sp+a*a; } sp = sqrt(sp); return(sp); } double Vektor::unorm() { double erg=0; for (int i=0; i<dimension ;i++) { if (fabs(erg)<=fabs(a)) { erg=fabs(a); } 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 * w->a; } return scalar; } thnx

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

Your operator=(Vektor v) will cause a buffer overflow if v.getdim() is bigger than this->getdim().

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 = other.a;}

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*.)

ok thanx a lot folks. Helped me a lot :-)