Destructor causes error upon delete doub

Forum

Forum
Beginners
Destructor causes error upon delete doub
Page 2

Destructor causes error upon delete double array

Pages: 123

> Yeah... nice so now we know the delete[] data; gets called twice for the same data? That probably causes the error?

Ok, good. Now make sure the most vital function is good and running.

void operator = (const VEC &v)
{ 
      // Log("\nOriginal dim == ", v.dim);

      dim = v.dim;
      data = v.data;
      if(data) memcpy((data = new double[dim]), v.data, sizeof(double) * dim); 

      Log("\n\nOriginal (v.data)'s address == ", v.data);
      Log("\nNew (data)'s address == ", data);
      Log("\nCopy operation successful! ", 0);
}

Last edited on

Kaisky (33)


	void operator = (const VEC &v)
	{
		Log(" '=' get called!");
		dim = v.dim;
		data = v.data;
		if (data) memcpy((data = new double[dim]), v.data, sizeof(double) * dim);

		Log("\nOriginal (v.data)'s address", v.data);
		Log("New (data)'s address", data);
		Log("\nCopy operation successful! ");
	}

the log file stays absolute empty, so it doesnt get called at all before the breakpoint :(

Naughty Albatross (1283)

Ah, how shame. You need to add a member function :

void copy(const VEC &v)
{ 
      // Log("\nOriginal dim == ", v.dim);      
dim = v.dim;
      data = v.data;
      if(data) memcpy((data = new double[dim]), v.data, sizeof(double) * dim); 
      Log("\n\nOriginal (v.data)'s address == ", v.data);
      Log("\nNew (data)'s address == ", data);
      Log("\nCopy operation successful! ", 0);
}

And with this line :
for(int i = 0; i < d; ++i) data[i] = data_[i];

You change it to :
for(int i = 0; i < d; ++i) data[i].copy(data_[i]); // don't use assignment, use a copy function instead

Edit: This line dim = v.dim; is not commented.

Last edited on

Kaisky (33)

i already had a copy void before, it looks like this:

// copies another vector into this one
void VEC::copy(VEC v) {
	if (v.dim != dim) {
		std::cout << "Error: can't copy a VEC of different dimension";
		return;
	}
	for (int i = 0; i < dim; ++i) {
		data[i] = v.get(i);
	}
}

can it stay like this or should i use your code? in our case all vec's have dim=2 so it should be ok.

and the line for(int i = 0; i < d; ++i) data[i] = data_[i]; doesnt even exist anymore, i deleted the constructer a while ago. So the copy function doesnt get called anyway...

Kaisky (33)

since the = assignment doesnt get called at all, i tried to log the address before and after in the destructor like this:

VEC::~VEC() {
	Log("\ndata address", data);
	if (data) {
		Log("\ndata is true. trying to delete[] data");
		delete[] data;
		data = NULL;
		dim = 0;
		Log("new data address", data);
	}
}

the output is:

data address = 00B9C960
data is true. trying to delete[] data
new data address = 00000000

data address = 00B9C960
data is true. trying to delete[] data

edit:
so for me it looks like the program creates two VEC's, both their data points to the same data[0], then they both get destroyed. but why does all this even happen before the WinMain() ? where does the compiler starting the code if not in the WinMain() function?

Last edited on

Naughty Albatross (1283)

Have you tried my copy function?

Anyway, let me know :
+ Your VEC global variables
+ Your VEC member declarations in other structs and classes
+ Did you use memcpy() (or similar functions) to copy VEC objects?
+ When assigning a VEC object to another VEC object, have you removed all assignment operators (=) and replaced them with VEC copy functions?

Thomas1965 (4571)

I downloaded your file, however I can't compile it because some header files are missing. I guess DirectX is a seperate download. However I had a closer look and your copy assignment looks a bit dodgy and the copy constructor is missing.

Try this:

VEC& operator = (VEC &v)
{
   dim = v.dim;
   if (data)
      delete[] data;
   data = new double[dim];
   for (int i = 0; i < dim; ++i)
      data[i] = v.get (i);

    return *this;
}

VEC (VEC& v)
{
   dim = v.dim;
   data = new double[dim];
   for (int i = 0; i < dim; ++i)
      data[i] = v.get (i);
}

Kaisky (33)

I hope i understand all your questions right, im not sure yet about all technical terms you use to describe.

> Your VEC global variables
as far as i see have only this

VEC const GRAVITY = Prdct(-0.00001, uV(1, 2));

> Your VEC member declarations in other structs and classes
im not sure what you mean, correct me if im wrong.
I dont have any structs and with VEC and two other classes, MAT (if VEC is a vector, MAT is a matrix.) and RECT (a physical object that gets rendered and uses VEC's and MAT's as is physics)
they look like this:

class MAT {
public:
	MAT();
	MAT(int, int);
	MAT(int, int, double*);
	//~MAT(); for MAT's i have the same problem as in VECs. but since VEC's dont use MATS its not caused here... getting the solution for VEC's should work for MAT's too.
	double get(int, int);
	void set(int, int, double);
	void copy(MAT);
	void add(MAT);
	void subtract(MAT);
	void mult(double);
	void mult(MAT);
	void mult(MAT, bool);
	void print();
	int dimX, dimY;
	bool quadratic;
private:
	double* data;
};

and the RECT:

class Rect {
public:
	Rect();
	Rect(int, double, double, double, double, double, double, double, double);
	~Rect();
	void WriteRectEdges(float*);
	void InitEdges();
	void UpdateEdges();
	void SetInactive();
	double GetMass();
	void DoLogic1();
	void DoLogic2();
	void DoLogic3();
	void BounceEdge(VEC, VEC, VEC);
	void Impact(VEC, VEC);
	void ImpactLocal(VEC, VEC);
private:
	bool active = false;
	int index;
	double w, h, angle, vel_angle;
	double mass;
	double momentum_of_inertia;
	int AMOUNT_OF_EDGES;
	VEC pos;
	VEC vel;
	VEC normalized_edges[4];
	VEC edg[4];
};

> Did you use memcpy() (or similar functions) to copy VEC objects?
no, not at all.
all my functions that use VEC, like "Sum(VEC, VEC)" always creates a new VEC as the sum of the other 2,
and the void's declared in the class like "add(VEC)" adds another vector do this one, so it just overwrites its data. but they dont get called before the crash, i logged all of them.

> When assigning a VEC object to another VEC object, have you removed all assignment operators (=) and replaced them with VEC copy functions?
I think I never used "=" at all for VEC's.
edit: from this i excluded when i define new VECs with '='.. does this count as well? for example the GRAVITY above.

I hope thats all the information you need, if not let me know.

edit2: your copy function didnt change anything. like i said, it doesnt get called anyway, my Log function works 100%. (at least this..)

@Thomas1965 i think we already tried the '=' thingy. and yeah you need the directx 11 sdk

Last edited on

Naughty Albatross (1283)

> Your VEC member declarations in other structs and classes
That means I want to know if VEC appear in other structs and classes. Your RECT class contains lots of them.

VEC pos;
	VEC vel;
	VEC normalized_edges[4];
	VEC edg[4];

And this line, I recommend you disable it :
const VEC GRAVITY;

The problem is now much narrowed down. If you just left your VEC objects alone and didn't call the second constructor (instead of the default one), the whole thing wouldn't happen.

So I want you to list all the lines that depict you initializing your VEC objects with the second constructor. For example :
VEC myvec(10);

VEC myvec; // Default constructor - should eliminate the problem

Make changes so that all your VEC objects initialize themselves with their default constructor only. Also, you should really add a copy constructor. This :

VEC(const VEC &v)
{ 
      Log("\nCopy constructor called!!");
      dim = v.dim;
      data = v.data;
      if(data)memcpy((data = new double[dim]), v.data, sizeof(double) * dim); 
}

Don't worry, you can do it. Let me know your program output outcome.

Edit : Fixed a small typo. The code should be VEC(const VEC &v)

Last edited on

Kaisky (33)

thank you for your patience.
i think i just was able to isolate the problem - just figured it out while you posted this:
the problem is indeed caused for the first time by the VEC GRAVITY.
i logged the functons that construct GRAVITY:


// returns a nullvector with dimension n
VEC emptyVec(int n) {
	VEC r(n);
	for (int i = 0; i < n; ++i) {
		r.set(i, 0);
	}
	Log("\n end of emptyVec");
	return r;
}

// returns the unit vector with dimension n in the i'th dimension
VEC uV(int i, int n) {
	Log("\n start of uV");
	VEC r = emptyVec(n);
	r.set(i, 1);
	Log("\n end of uV");
	return r;
}


// multiplies a scalar with a vector
VEC Prdct(double a, VEC v) {
	Log("start of Prdct");
	const int dim = v.dim;
	VEC r(dim);
	for (int i = 0; i < dim; ++i) {
		r.set(i, v.get(i) * a);
	}
	Log("end of Prdct");
	return r;
}

and got this:


 start of uV
 end of emptyVec

data address = 008CCA40
new data address = 00000000
 end of uV

data address = 008CCA40

uV still gets called completly, but it crashes upon giving the VEC to Prdct in
VEC const GRAVITY = Prdct(-0.00001, uV(1, 2))

edit: should i still try to do what you said?

Last edited on

Kaisky (33)

its weird, even after

VEC r = emptyVec(n);

the

Log("\n '=' gets called!");

doesnt happen in

void operator = (const VEC &v)

edit: I'm rewriting everything now to simplify the code as much as possible in a seperate project and then attempting to do what you said in your last post. this might take me an hour.

Last edited on

Naughty Albatross (1283)

> I think i just was able to isolate the problem - just figured it out

Congratulations!

> Its weird, even after
VEC r = emptyVec(n);
the Log("\n '=' gets called!"); doesnt happen

You must add a new assignment constructor. It may look the same, but this ctor is for (VEC v) not (VEC &v)

void operator = (const VEC v)
{ 
      Log("\nAssignment constructor (VEC) called!!"); 
    if(data)this->~VEC();
     dim = v.dim;
      data = v.data;
      if(data) memcpy((data = new double[dim]), v.data, sizeof(double) * dim); 
}

Let's make some little changes to your functions :

VEC emptyVec(int n) 
{
	VEC r(n);
	for (int i = 0; i < n; ++i) {
		r.set(i, 0);
	}
	Log("\n end of emptyVec");

 return VEC(r);
}

VEC uV(int i, int n) 
{
	Log("\n start of uV");	
    VEC r(emptyVec(n));
	r.set(i, 1);
	Log("\n end of uV");
    return VEC(r);
}

VEC Prdct(double a, VEC v) {
	Log("start of Prdct");
	const int dim = v.dim;
	VEC r(dim);
	for (int i = 0; i < dim; ++i) {
		r.set(i, v.get(i) * a);
	}
	Log("end of Prdct");
	return VEC(r);
}

You should at least receive several "copy constructor called" messages. Now you enable this line :
const VEC GRAVITY(Prdct(-0.00001, uV(1, 2)));

...and let me know if this works.

Last edited on

Naughty Albatross (1283)

Also you should add a copy constructor for (VEC v) as well :

VEC(const VEC v)
{ 
      Log("\nCopy constructor (VEC) called!!"); 
    if(data)this->~VEC();
     dim = v.dim;
      data = v.data;
      if(data) memcpy((data = new double[dim]), v.data, sizeof(double) * dim); 
}

Naughty Albatross (1283)

Check if the following constructors or functions are missing :

VEC(const VEC v);
VEC(const VEC &v);
void operator =(const VEC v);
void operator =(const VEC &v);

Last edited on

Kaisky (33)

Ok, heres the whole new project.
I get an error for your function

	// copy constructor for v
	VEC(const VEC v) {
		std::cout << "\n Copy constructor (VEC) called!!";
		if (data)this->~VEC();
		dim = v.dim;
		data = v.data;
		if (data) memcpy((data = new double[dim]), v.data, sizeof(double) * dim);
	}

:
this one:

1>d:\visual studio projects\projects\threadtest\threadtest\main.cpp(26): error C2652: 'VEC': illegal copy constructor: first parameter must not be a 'VEC'
1>  d:\visual studio projects\projects\threadtest\threadtest\main.cpp(5): note: see declaration of 'VEC'
1>d:\visual studio projects\projects\threadtest\threadtest\main.cpp(26): error C2333: 'VEC::VEC': error in function declaration; skipping function body
1>d:\visual studio projects\projects\threadtest\threadtest\main.cpp(78): warning C4521: 'VEC': multiple copy constructors specified
1>d:\visual studio projects\projects\threadtest\threadtest\main.cpp(78): warning C4522: 'VEC': multiple assignment operators specified
1>d:\visual studio projects\projects\threadtest\threadtest\main.cpp(109): error C2664: 'VEC Product(double,VEC)': cannot convert argument 2 from 'VEC' to 'VEC'
1>  d:\visual studio projects\projects\threadtest\threadtest\main.cpp(109): note: Cannot copy construct class 'VEC' due to ambiguous copy constructors or no available copy constructor

besides, i put everything in a whole new project. you should be able to compile it (except the one error now...)

#include <iostream>

class VEC;

class VEC {
public:
	// default constructor
	VEC() {}

	// dimensions constructor
	VEC(int d) {
		dim = d;
		data = new double[d];
	}

	// copy constructor for &v
	VEC(const VEC &v) {
		std::cout << "\n Copy constructor (VEC) called!!";
		if (data)this->~VEC();
		dim = v.dim;
		data = v.data;
		if (data) memcpy((data = new double[dim]), v.data, sizeof(double) * dim);
	}

	// copy constructor for v
	VEC(const VEC v) {
		std::cout << "\n Copy constructor (VEC) called!!";
		if (data)this->~VEC();
		dim = v.dim;
		data = v.data;
		if (data) memcpy((data = new double[dim]), v.data, sizeof(double) * dim);
	}

	// ssignment constructor for &v
	void operator = (const VEC &v)
	{
		std::cout << "\n Assignment constructor (VEC) called!!";
		if (data)this->~VEC();
		dim = v.dim;
		data = v.data;
		if (data) memcpy((data = new double[dim]), v.data, sizeof(double) * dim);
	}

	// ssignment constructor for v
	void operator = (const VEC v)
	{
		std::cout << "\n Assignment constructor (VEC) called!!";
		if (data)this->~VEC();
		dim = v.dim;
		data = v.data;
		if (data) memcpy((data = new double[dim]), v.data, sizeof(double) * dim);
	}

	// destructor
	~VEC() {
		std::cout << "\n ~VEC got called. checking if data is true.";
		if (data) {
			std::cout << "\n data is true. attempting to delete data at address " << data;
			delete[] data;
			std::cout << "\n deletion succesful";
		}
		else std::cout << "\n data is false.";
	}

	// copy
	void copy(const VEC &v)
	{  
		std::cout << "\n attempting to copy!";
		dim = v.dim;
		data = v.data;
		if (data) memcpy((data = new double[dim]), v.data, sizeof(double) * dim);
		std::cout << "\n Original (v.data)'s address == " << v.data;
		std::cout << "\n New (data)'s address == " << data;
		std::cout << "\n Copy operation successful! ";
	}
	int dim;
	double* data;
};

VEC Product(double x, VEC v) {
	int const dim = v.dim;
	VEC r(dim);
	for (int i = 0; i < v.dim; ++i) {
		r.data[i] = v.data[i] * x;
	}
	return VEC(r);
}

VEC EmptyVector(int n) {
	VEC r(n);
	std::cout << "\n empty vec start. \n vec address == " << &r << " and its data address == " << r.data;
	for (int i = 0; i < n; ++i) {
		r.data[i] = 0;
	}
	return VEC(r);
}

VEC UnitVector(int i, int n) {
	VEC r;
	std::cout << "\n unit vec start. \n vec address == " << &r << " and its data address == " << r.data;
	r.copy(EmptyVector(n));
	std::cout << "\n unit vec after copy. \n vec address == " << &r << " and its data address == " << r.data;
	r.data[i] = 1;
	return VEC(r);
}

int main() {
	std::cout << "\n attempting to calculate the gravity";
	VEC gravity = Product(5, UnitVector(1, 2));
	std::cout << "\n gravity vector calculation complete.";
	int pause = 0;
	std::cin >> pause;
	return 0;
}

edit: changed all

return r;

return VEC(r);

but it didnt change the output.

Last edited on

Naughty Albatross (1283)

Ok, you can remove this function :
VEC(const VEC v);

Let me know if your project is free of compiler errors.

Kaisky (33)

yes it is.
the project is now http://pastebin.com/JVf8uVZe
the console output is:

 attempting to calculate the gravity
 unit vec start.
 vec address == 00AFF948 and its data address == 00000000
 empty vec start.
 vec address == 00AFF824 and its data address == 00BE3B88
 Copy constructor (VEC) called!!
 ~VEC got called. checking if data is true.
 data is true. attempting to delete data at address CCCCCCCC

Last edited on

Naughty Albatross (1283)

You forgot to initialize members in your default ctor again :
// Default constructor VEC() {data = NULL; dim = 0;}

Thomas1965 (4571)

Try this:

#include <iostream>

class VEC
{
public:
  // default constructor
  VEC () 
  {
    dim = 0;
    data = 0;
  }

  // dimensions constructor
  VEC (int d) 
  {
    dim = d;
    data = new double[d];
  }

  // copy constructor for &v
  VEC (const VEC &v) 
  {
    std::cout << "\n Copy constructor (VEC&) called!!";
      
    dim = v.dim;
    data = new double[dim];
    memcpy (data, v.data, sizeof (double) * dim);
  }

  // assignment constructor for &v
  VEC& operator = (const VEC &v)
  {
    std::cout << "\n Assignment constructor (VEC) called!!";
    if (data)
      delete[] data;
    dim = v.dim;
    data = new double[dim];
    memcpy(data, v.data, sizeof (double) * dim);
  }

  // destructor
  ~VEC () 
  {
    std::cout << "\n ~VEC got called. checking if data is true.";
    if (data)
    {
      std::cout << "\n data is true. attempting to delete data at address " << data;
      delete[] data;
      std::cout << "\n deletion succesful";
    }
    else std::cout << "\n data is false.";
  }

  // copy
  void copy (const VEC &v)
  {
    std::cout << "\n attempting to copy!";
    dim = v.dim;
    
    if (data)
      delete[] data;
    dim = v.dim;
    data = new double[dim];
    memcpy (data, v.data, sizeof (double) * dim);
    std::cout << "\n Original (v.data)'s address == " << v.data;
    std::cout << "\n New (data)'s address == " << data;
    std::cout << "\n Copy operation successful! ";
  }
  int dim;
  double* data;
};

VEC Product (double x, VEC v) 
{
  int const dim = v.dim;
  VEC r (dim);
  for (int i = 0; i < v.dim; ++i)
  {
    r.data[i] = v.data[i] * x;
  }
  return r;
}

VEC EmptyVector (int n) {
  VEC r (n);
  std::cout << "\n empty vec start. \n vec address == " << &r << " and its data address == " << r.data;
  for (int i = 0; i < n; ++i)
  {
    r.data[i] = 0;
  }
  return r;
}

VEC UnitVector (int i, int n) {
  VEC r;
  std::cout << "\n unit vec start. \n vec address == " << &r << " and its data address == " << r.data;
  r.copy (EmptyVector (n));
  std::cout << "\n unit vec after copy. \n vec address == " << &r << " and its data address == " << r.data;
  r.data[i] = 1;
  return r;
};

bool main () {
  std::cout << "\n attempting to calculate the gravity";
  VEC gravity = Product (5, UnitVector (1, 2));
  std::cout << "\n gravity vector calculation complete.";
  
  system ("pause");
  return 0;
}

Output:


 attempting to calculate the gravity
 unit vec start.
 vec address == 004DFDA8 and its data address == 00000000
 empty vec start.
 vec address == 004DFD5C and its data address == 005CC118
 Copy constructor (VEC&) called!!
 ~VEC got called. checking if data is true.
 data is true. attempting to delete data at address 005CC118
 deletion succesful
 attempting to copy!
 Original (v.data)'s address == 005CC168
 New (data)'s address == 005CC118
 Copy operation successful!
 ~VEC got called. checking if data is true.
 data is true. attempting to delete data at address 005CC168
 deletion succesful
 unit vec after copy.
 vec address == 004DFDA8 and its data address == 005CC118
 Copy constructor (VEC&) called!!
 ~VEC got called. checking if data is true.
 data is true. attempting to delete data at address 005CC118
 deletion succesful
 Copy constructor (VEC&) called!!
 ~VEC got called. checking if data is true.
 data is true. attempting to delete data at address 005CC118
 deletion succesful
 ~VEC got called. checking if data is true.
 data is true. attempting to delete data at address 005CC168
 deletion succesful
 gravity vector calculation complete.Press any key to continue . . .

Kaisky (33)

oh nice... it all works now!
thank you closed account 5a8Ym39o6
and thank you thomas1965 !
i think i'll try to build my whole project from the beginning, at least the "MATHS.h" file.
learned so much about memory management now that i feel like i have to change everything again.

I think from here on i'll be able to do everythign on my own .. (until i need help for sthing else, but then i'll make new topic)

would you recommend to use vectors instead of arrays for the data?
i think i read somewhere that vectors are better for adding and deleting elements at the end, while arrays are better for going through all elements.
since my VEC and MAT classes rather go through all elements and never change their dimension, arrays are the better choice right?

And can i use structs instead of classes? are they better for performance? (since i attempt to make a really complex physics engine, because im physics student)

and if structs are "faster" than classes, can i still do all the things i did with classes, like read/change their data and only read their dimension?

regards, Kaisky

Last edited on

Pages: 123

C++

Forum

Destructor causes error upon delete double array