Program ends at the end of the constructor

[Alpacaman]

Hello everyone, I've read many posts of crashing constructors but all of them crashes somewhere in the middle of it, mine crashes after finishing the constructor, and it happens just with the constructor with arguments. Any idea of what could be happening here?

Some code explanation:
The program is just creating a new Mesh, the mesh contains Squares but when it is created is just one square, the square is created directly in the constructor of the class Mesh.

Main:

#include <iostream>
#include "Mesh.h"
#include "Square.h"
#include "point.h"
using namespace std;

int main() {
	cout << "Why, Hello there" << endl;
	int len=10;
	int wid=5;
	Mesh *nueva=new Mesh;
	cout<<"Memory allocated"<<endl;
	*nueva=Mesh(len,wid);
	cout<<"Mesh Created"<<endl;
	return 0;
}

Class Mesh

#ifndef MESH_H_
#define MESH_H_
#include <iostream>
#include <vector>
#include "Point.h"
#include "Square.h"

class Mesh
{
private:
	int lenght,width,noSquares;
	int noSq;
	Square *squares;
	int noPts;
	Point *points;

public:
	Mesh()
	{
		lenght=width=noSquares=0;
		noPts=4;
		points = new Point[noPts];
		noSq=1;
		squares = new Square[noSq];
		points[0]=*(squares[0].tellP1());
		points[1]=*(squares[0].tellP2());
		points[2]=*(squares[0].tellP3());
		points[3]=*(squares[0].tellP4());
		// Using this constructor the program does not crash
	}
	Mesh(int l, int w)
        {
		noPts=4;
		points = new Point[noPts];
		noSq=1;
		squares = new Square[noSq];
		lenght=l;
		width=w;
		noSquares=0;
		Point p1(0,l,0);
		Point p2(0,0,0);
		Point p3(w,0,0);
		Point p4(w,l,0);
		points[0]=p1;
		points[1]=p2;
		points[2]=p3;
		points[3]=p4;
		Square waa(points,(points+1),(points+2),(points+3));
		squares[0]=waa;
		noSquares++;
		squares[0].printSquare();
		//After printing the square the program crashes
	}

	~Mesh()
	{
		delete &lenght;
		delete &width;
		delete &noSquares;
		delete[] squares;
		delete[] points;
		squares=0;
		points=0;
	}
};

#endif /* MESH_H_ */ 

Program Output:

Why, Hello there
Memory allocated
(0,10,0)  (5,10,0)
(0,0,0)  (5,0,0)

Im also sure that it is not the method printSquare() I've tested it and it works fine.

[K0T4K0t4]

You usually wouldn't want to do something like
squares[0].printSquare();
inside of a constructor.
if you do that call after the constructor is finished you should be fine i think.

[Alpacaman]

I put that there to see if the constructor reach the end, and it does, so I don't know why it is crashing. I already removed that and keeps crashing.

[cire]

Your class needs a copy assignment operator and a copy constructor.

The compiler-generated copy assignment operator is invoked on line 13 of main, and it isn't up to the task.

[ne555]

Or remove all that dynamic allocation and be happy

> I put that there to see if the constructor reach the end,
http://betterexplained.com/articles/debugging-with-gdb/i


delete &lenght; don't delete what you didn't new

[Disch]

Your understanding of constructors is incorrect.

	Mesh *nueva=new Mesh;  // <- this allocates AND CREATES the object.
       //  It uses the default constructor
	cout<<"Memory allocated"<<endl;
	*nueva=Mesh(len,wid);  // <- this creates ANOTHER "temporary" object, then assigns
       //  that temporary object to nueva
	cout<<"Mesh Created"<<endl;

This is problematic because then the temporary object is immediately destroyed, which destroys the allocated buffers in your Mesh object (since you only did a shallow copy).

What you're doing is basically this:

Mesh nueva;  // allocates and create a mesh
Mesh temporary(len,wid);  // allocate and create another mesh
nueva = temporary;  // move it over
// <- 'temporary' is destroyed here 

This is not what you want.



Solutions + advice:

1) Don't use new unless you have to (ie: unless polymorphism is involved or you need to have a lifetime larger than the scope the object is created in). If you do not absolutely need to use new, it's best to avoid it.

2) There's a rule called the "rule of 3". Which basically says that if you need to implement the copy constructor, assignment operator, or the destructor.... then you probably need to implement all 3 of them. Here you are implementing the destructor to clean up your mesh data... so this is true for you. You will also need to implement the copy constructor and assignment operator so they do deep copies to avoid the problem you're having.



3) .... but really... the best solution is to not implement the destructor... and use container classes like vector instead of doing the allocation yourself.

4) never... EVER, EVER EVER EVER delete something you didn't 'new'. You did not allocate 'length', 'width' or 'noSquares' with new, so you ABSOLUTELY MUST NOT delete them.



Your code, fixed & improved. See my comments:

//main
#include <iostream>
#include "Mesh.h"
#include "Square.h"
#include "point.h"
using namespace std;

int main() {
	cout << "Why, Hello there" << endl;
	int len=10;
	int wid=5;
	Mesh nueva(10,5);  // <- no need for new.  Just create the object here
	cout<<"Memory allocated & Mesh Created"<<endl;
	return 0;
}

//mesh.h
#ifndef MESH_H_
#define MESH_H_
#include <iostream>
#include <vector>
#include "Point.h"
#include "Square.h"

class Mesh
{
private:
	int lenght,width;   // not sure why you had 'noSq' and 'noSquares'
	std::vector<Square> squares;  // just use a vector to keep track of your squares
		// do not dynamically allocate them.

	std::vector<Point> points;  // ditto for points

public:
	Mesh()
	{
		lenght=width=0;

//		noPts=4;          // instead of doing this... 
//		points = new Point[noPts];
		points.resize(4);   // just do this

//		noSq=1;     // and instead of this
//		squares = new Square[noSq];
		squares.resize(1);   // just do this

		// this works the same as before
		points[0]=*(squares[0].tellP1());
		points[1]=*(squares[0].tellP2());
		points[2]=*(squares[0].tellP3());
		points[3]=*(squares[0].tellP4());
	}
	Mesh(int l, int w)
        {
//		noPts=4;     again... ditch this crap
//		points = new Point[noPts];
//		noSq=1;
//		squares = new Square[noSq];
		points.resize(4);
		squares.resize(1);

		lenght=l;
		width=w;

/*    this can be shortened
		Point p1(0,l,0);
		Point p2(0,0,0);
		Point p3(w,0,0);
		Point p4(w,l,0);
		points[0]=p1;
		points[1]=p2;
		points[2]=p3;
		points[3]=p4;
*/
		points[0] = Point(0,l,0);  // although it would be better to call a
		points[1] = Point(0,0,0);   //  "set" style function if there's one available
		points[2] = Point(w,0,0);
		points[3] = Point(w,l,0);

		// this also works the same... but I'm weary on your use of pointers here....
		Square waa(points,(points+1),(points+2),(points+3));
		squares[0]=waa;

		squares[0].printSquare();
	}

	/*   Get rid of this entirely.  vector cleans up automatically.  If you don't 'new'
		then there's nothing to delete
	~Mesh()
	{
		delete &lenght;
		delete &width;
		delete &noSquares;
		delete[] squares;
		delete[] points;
		squares=0;
		points=0;
	}*/
};

#endif /* MESH_H_ */  

[AbstractionAnon]

Or did you mean for the following at line 13?

nueva= new Mesh(len,wid);

Regardless of whether you meant new or an implicit copy as cire suggested, you have a memory leak. Your default constructor allocates points and squares. Both new and the implicit copy overwrite points and squares without releasing the previously allocated memory.

[Alpacaman]

THANK YOU GUYS! I followed Disch instructions and now it runs as it should, I also removed destructor in other classes. I'll read more about Constructors and Destructors as I can see now I don't fully understand them.

Again, thank you guys.