C++ copy constructor trouble =(

[Craig Pemberton]

I'm having trouble getting my code to work properly.

I have two classes: Vertex, which represents a vertex in 3-space, and Matrix, which is a 2d array of vertices.

My Matrix copy constructor causes a segfault in the Vertex assignment operator.

I'm not very well versed in c++ so a lot of this stuff is hacked out rather naively. Any help would be greatly appreciated!

Best regards.
Craig

/* data.cpp
 * The data structure which will represents the points of our surface.
 */
#include <GL/gl.h>
#include <vector>
#include <stdio.h>
#include <stdlib.h>
using namespace std;

// This class represents a vertex in 3-space
class Vertex
{
	private:// The coordinates of a vertex in 3-space
		float x, y, z;
	public:
		// Default constructor
		Vertex(void):x(0.0), y(0.0), z(0.0) {}

		// Parameterized constructor
		Vertex(float x_, float y_, float z_)
		{
			x = x_;
			y = y_;
			z = z_;
		}

		// Destructor
		~Vertex() { /* nothing on the heap to free */ }

		// Copy constructor
		Vertex(const Vertex& v)
		{
			x = v.x;
			y = v.y;
			z = v.z;
		}

		// Assignment operator
		void operator=(const Vertex& v) 
		{
			x = v.x;
			y = v.y;
			z = v.z;
		}

		// Print
		void print()
		{
			printf("<%1.1f,%1.1f,%1.1f>", x, y, z);
		}

		// Tell openGL to render this vertex
		// Call only between glBegin(*) and glEnd()!
		void draw()
		{
			glVertex3f(x, y, z);
		}

		//Vertex-vertex addition
		Vertex operator+(Vertex v)
		{
			return Vertex(x+v.x, y+v.y, z+v.z); 
		}

		//Vertex-scalar multiplication
		Vertex operator*(float f)
		{
			return Vertex(f*x, f*y, f*z); 
		}
};

// This class represents a row-major 2D "grid" of vertices
class Matrix
{
	private:// Dimensons of the matrix
		int rows, cols;
		// STL vector of vectors of Verticies
		vector< vector<Vertex> > data;

	public:	// Constructor
		Matrix(int rows_, int cols_)
		{
			rows  = rows_;
			cols = cols_;
			for(int r=0; r<rows; r++)
			{
				data.push_back(vector<Vertex>());
				for(int c=0; c<cols; c++)
				{
					data.back().push_back(Vertex());
				}
			}
		}

		// Destructor
		~Matrix() //this won't work for some reason =(
		{
			//vector<Vertex>::iterator i;
			//for(i = data.begin(); i != data.end(); i++) i.clear();
		}

		// Copy constructor
		Matrix(const Matrix& old)
		{
			rows = old.rows;
			cols = old.cols;
			for(int r=0; r<rows; r++)
			{
				data.push_back(vector<Vertex>());
				for(int c=0; c<cols; c++)
				{
					Vertex temp = old.data[r][c]; //segfault!
					data.back().push_back(temp);
				}
			}
		}

		// Print
		void print()
		{
			// Should use iterators instead but I can't get them to work =(
			for(int r=0; r<rows; r++)
			{
				for(int c=0; r<cols; c++)
				{
					data[r][c].print();
				}
				printf("\n");
			}
		}

		// Access cols and rows
		int numCols()	{ return cols; }
		int numRows()	{ return rows; }

		// v returns an element of the matrix by reference
		Vertex& v(int r, int c)
		{
			if( 0 > r || r > rows || 0 > c || c > cols)
			{
				printf("Matrix boundary error!\nr:%i rows:%i, c:%i, cols:%i\n", r, rows, c, cols);
				exit(-1);
			}
			return data[r][c];
		}

};

[helios]

Uh... You don't actually need to define your own copy constructor for Matrix. The compiler-defined one will do copy.data=original.data. This will copy the vector of vectors of Vectors by itself.
The copy constructor for Vector and the operator= overload are also unnecessary.

[Craig Pemberton]

I don't want my new object to point to the same data as the original object. I want to do a deep copy.

[Disch]

But these classes don't have any pointers, so you don't have to worry about making a deep copy.

The default copy ctors and assignment operators would work fine for both of these classes.

[manojr]

How are you putting the vertex data in the matrix? I can see constructor, but where is feeling the data code?

[imi]

Another hint: Your constructor of Matrix can be simplified alot. It's now:

		Matrix(int rows_, int cols_)
		{
			rows  = rows_;
			cols = cols_;
			for(int r=0; r<rows; r++)
			{
				data.push_back(vector<Vertex>());
				for(int c=0; c<cols; c++)
				{
					data.back().push_back(Vertex());
				}
			}
		}

Make it:

		Matrix(int rows_, int cols_)
			: rows(rows_)
			, cols(cols_)
			, data(rows_, vector<Vertex>(cols_))
		{
		}

std::vector has a copy-semantic, which means it holds copies of all values you put into it (and move them around when necessary). This is the reason, you won't get problems with aliasing (two matrices pointing to the same vertex).


Ciao, Imi.

[Craig Pemberton]

Thank you imi! That helps me a lot. =)

Does the standard destructor also work properly as expected or do I need to do something?

Manojr: The paradigm I'm using for my data goes like:

Matrix m = Matrix(rows, cols);
m.v(r,c).print();
m.v(r,c) = Vertex(3.5, 4.0, 0.0) *5.0;
m.v(r,c).print();
glBegin(GL_POINTS);
m.v(r,c).draw();
glEnd();
. . . .
 

Maybe that gives you a general idea of how I'm using the data.

[imi]

vector frees its own memory, and since you didn't allocate anything for the things you stuff into the vector (they are all plain int's), you don't need to do anything to get rid of a Matrix.

You can even delete the whole destructor.

		~Matrix() //this won't work for some reason =(
		{
			//vector<Vertex>::iterator i;
			//for(i = data.begin(); i != data.end(); i++) i.clear();
		}

The reason this won't work is, that "clear" is a member function of vector, not of an iterator to Vertex.

You *could* call "data.clear()" in your destructor, but it's kind of useless, as your destructor is the last thing you will see of the Matrix anyway and just after your destructor finished, C++ will call the destructor of all members, including data. And guess what? Yes, the destructor of vector calls clear() (or at least it does the same that clear would do: destructing all its elements).

Ciao, Imi.