Home
Blogs
Forums
News
Portfolios
Projects
Tutorials
40 users logged in
New? Learn about game development.
Before posting, review our community guidelines.
Follow Us
Chat in the GameDev.net Discord!
Mastodon
Back to General and Gameplay Programming

PROBLEM: std::vector and pointers

Started by
12 comments, last by Glak 18 years, 7 months ago
Hi all, This is hard to explain in words so i'll give a code example: Please check out this class: 

class SomeClass
{
public:
	SomeClass()
	{
		SomePointer = NULL;
	}
	SomeClass()
	{
		if (SomePointer) delete SomePointer;
	}
	void InitPointer()
	{
		SomePointer = new char[20];
	}
private:
	char *SomePointer;
}
The code below will keep a vector that stores instances of SomeClass. When I have pointers to certain instances and push_back() these instances into the vector and delete the original instance SomePointer will be deleted aswell which gives me a problem. It would be very very bad code when I don't delete the pointer in the dtor of SomeClass. Anyway, here's the code that would mess the SomePointer up in the copied SomeClasses in the vector: 

std::vector<SomeClass> Vector;

SomeClass *A = new SomeClass;
A->InitPointer();
Vector.push_back(*A);
delete A;
What would be the right way to handle this? Thanx a lot! Marty
_____ /____ /|| | || MtY | ||_____|/Marty
Advertisement
Copy Constructor!

All classes that are used in a Standard C++ Library container need a copy constructor. Basically, it is used when created a new instance from an old one. This happens when you insert a new item into a container (the new item gets copied, and the copy goes into the container), when the container needs to shift stuff around in memory (a copy gets created in the new location, and the original then gets removed), etcetera. Make your code look something like this:

class SomeClass{public:	SomeClass()	{		SomePointer = NULL;	}	//COPY CONSTRUCTOR	SomeClass(const SomeClass& Instance)	{		if (Instance.SomePointer)		{			InitPointer();			memcpy(SomePointer, Instance.SomePointer, 20 * sizeof(char));		}		else		{			SomePointer = NULL;		}	}	~SomeClass()	{		if (SomePointer) delete SomePointer;	}	void InitPointer()	{		SomePointer = new char[20];	}private:	char *SomePointer;}


Given that this is obviously example code, I won't harp much on other things that could be improved (initialization list for constructors, using std::string instead of char[20]), but I thought I'd throw a few out their just in case they'd be worth mentioning.
"We should have a great fewer disputes in the world if words were taken for what they are, the signs of our ideas only, and not for things themselves." - John Locke 
class SomeClass{	private:		std::string data_;};

or
class SomeClass{	private:		std::vector< something > data_;};

or
class SomeClass{	private:		boost::shared_array< something > data_;};

or
std::vector< boost::shared_ptr< SomeClass > > vector;vector.push_back(boost::shared_ptr< SomeClass >(new SomeClass()));

Enigma
You should try to keep a clear idea of ownership for all pointers.
If data is shared, and doesn't have any such clear ownership, then you could either let the pointers manage themselves through some auto/shared pointer, or look into alternative ways of storing your data.

In your example, you would probably want to use a string class, such the one in the standard library.
Enigma probably has the right idea.

On the other hand, if the array size you want is known and constant like in your example, there's no need for a pointer. If you declare an array member instead, it will be copied across when the objects are copied. (What goes wrong with the vector usage is that std::vector implicitly copies your objects at various times.)

On the other other hand, if you are sure you have a really good reason not to use a real string type (e.g. it really is a data buffer type instead) *and* you have a good reason not to just use an array (e.g. later the desired size will come from outside, or you really need to delay instantiation of the array for some weird reason), then you will want to implement a copy constructor. Oh, and an assignment operator, too. [google] "C++ rule of three"; if you need any of (copy constructor, assignment operator, destructor), you generally need them all.

class SomeClass {  int size; // amount of allocated space (because you'll want to know later)  char* SomePointer;  public:  // normal ctor.  // The InitPointer approach as written is probably a time bomb; think what  // happens if you called it twice on the same object, or forgot the call.  // A zero-size allocation is OK, so let's do this:  SomeClass(int size = 0) : size(size), SomePointer(new char[size]) {}   // oh yeah, use initializer lists too!    // Now we do a copy constructor:  SomeClass(const SomeClass& other) : size(size), SomePointer(new char[other.size]) {    // OK, we made a new local allocation, now let's copy data to the new object    std::copy(SomePointer, SomePointer + size, other.SomePointer);  }  // And assignment operator, using copy-and-swap idiom for the allocation:  SomeClass& operator= (const SomeClass& rhs) {    SomeClass other(rhs);    std::swap(SomePointer, other.SomePointer);    size = other.size;  } // at end of scope, the 'other' gets destructed, and takes our old    // data allocation with it.  // Finally, the dtor.  ~SomeClass() { delete SomePointer; }  // By the way, you never have to check for null; 'delete NULL' is safe and  // is effectively a no-op. If the pointer will persist, though (i.e. not  // part of an object being destructed), you may want to *set* it NULL after  // deleting, though.}


But seriously, never mind all that and just do like Enigma says. :)
When you pass an object by value into a function a temporary bitwise copy of your object is created. Now your origional somePointer and the temporary objects somePointer point to the same memory. When push_back() returns, the temporary object's destructor is called and the memory is deallocated.

To fix this you would use a copy constructor. The copy constructor is called when you pass your object by value into a function or you return an object by value. If the copy constructor is not defined then a bitwise copy is created.
Hi!

Thanx for all the replies ppl. I think the copy constructors are a good solution, but it will be slow when copying lots of data many times. I made this example to be as simple as possible. That's why it's a char[20]. Normally it's a pointer to a temporary instance of a class in a .3ds file loader.

An example... The loader finds a new modelobject in the file. It creates a new cModelObject* and fills it in with the information it finds in the file (also the SomePointer that really is a pointer to a shared material class).
Then pushes the object onto the objects vector and delete the object pointer so the next time it finds an object it can make a new object and use the same pointer for the next new object.

This might just be a bad design, but it looked logical when i made it. Any suggestions?

Thanx a lot,
Marty

Sorry for having no comments in the code yet. 3ds files are very simple. They contain of chunks which all have a 6 byte header that tells the ID of the chunk and the size of the chunk. A chunk can contain subchunks.
anyways, here's the code:

cmodel* c3dsImporter::import(char *filename){	unsigned short ID;	unsigned int length;	fpos_t pos;	unsigned int mainend, i;	unsigned short num; // temp short	float f1, f2, f3; // temp floats	unsigned short us1, us2, us3, us4; // temp shorts		FILE * file = NULL;	s3dsModelObject *theObject;	s3dsMaterial *theMaterial;	s3dsFacesMaterial *theFacesMaterial;	theModel = new s3dsModel;		if ((file = fopen(filename, "rb")) == NULL) {return NULL;}	while (pos < mainend)	{		fread(&ID, 2, 1, file);		fread(&length, 4, 1, file);		switch (ID)		{		case CHUNK_MAIN:			printf("main chunk found\n");//////////			fgetpos(file, &pos);			mainend = pos + length - 6;			break;		case CHUNK_OBJMESH:			break;		case CHUNK_OBJBLOCK:			if (theObject)			{				theModel->objects.push_back(*theObject);				delete theObject;				theObject = new s3dsModelObject;			}			readString(theObject->name);			break;		case CHUNK_TRIMESH:			break;		case CHUNK_VERTLIST:			fread(&num, sizeof(unsigned short), 1, file);			for (i = 0; i < num; i++)			{				fread(&f1, sizeof(float), 1, file); 				fread(&f2, sizeof(float), 1, file);				fread(&f3, sizeof(float), 1, file);				theObject->verts.push_back(cvec3(f1, f2, f3));			}			break;		case CHUNK_FACELIST:			fread(&num, sizeof (unsigned short), 1, file);			for (i = 0; i < num; i++)			{				fread(&us1, sizeof(unsigned short), 1, file); 				fread(&us2, sizeof(unsigned short), 1, file);				fread(&us3, sizeof(unsigned short), 1, file);				fread(&us4, sizeof(unsigned short), 1, file);				theObject->faces.push_back(s3dsFace(us1, us2, us3, us4));			}			break;		case CHUNK_MAPLIST:			fread(&num, sizeof(unsigned short), 1, file);			for (i = 0; i < num; i++)			{				fread(&f1, sizeof(float), 1, file);				fread(&f2, sizeof(float), 1, file);				theObject->texCoords.push_back(s3dsUV(f1, f2));			}			break;		case CHUNK_FACEMAT:			theFacesMaterial = new s3dsFacesMaterial();			readString(theFacesMaterial->name);			fread(&num, sizeof(unsigned short), 1, file);			theFacesMaterial->faceNums = new unsigned short[num];			fread(&theFacesMaterial->faceNums, sizeof(unsigned short), num, file);			theObject->facesMaterial.push_back(*theFacesMaterial);			delete theFacesMaterial;			break;		case CHUNK_MATERIAL:			if (theMaterial) {theModel->materials.push_back(*theMaterial); delete theMaterial;}			theMaterial = new s3dsMaterial;			break;		case CHUNK_MATNAME:			readString(theMaterial->name);			break;		case CHUNK_AMBIENT:			readColor(&theMaterial->ambient);			break;		case CHUNK_DIFFUSE:			readColor(&theMaterial->diffuse);			break;		case CHUNK_SPECULAR:			readColor(&theMaterial->specular);			break;		case CHUNK_SHININESS:			readPercentage(&theMaterial->shininess);			theMaterial->shininess *= 128; // for ogl			break;		case CHUNK_TEXTURE:			readPercentage(&theMaterial->percentage);			fread(&ID, 2, 1, file);	fread(&length, 4, 1, file);			if (ID == CHUNK_MAPFILE) readString(theMaterial->texturemap);			else fseek(file, length-6, SEEK_CUR);			break;		case CHUNK_BUMPMAP:			readPercentage(&theMaterial->bumppercentage);			fread(&ID, 2, 1, file);	fread(&length, 4, 1, file);			if (ID == CHUNK_MAPFILE) readString(theMaterial->bumpmap);			else fseek(file, length-6, SEEK_CUR);			break;		default:			fseek(file, length-6, SEEK_CUR);			break;		}		fgetpos(file, &pos);	}	fclose(file);	if (theObject) {theModel->objects.push_back(*theObject); delete theObject;}	if (theMaterial) {theModel->materials.push_back(*theMaterial); delete theMaterial;}		calculateNormals();	cmodel *model = transformToModel();		delete theModel;	return model;}

_____ /____ /|| | || MtY | ||_____|/Marty
I don't want to be harsh, but what you have there is "C with classes", not C++. Streams? Exception safety? RAII? You could easily convert that to pure C and not lose anything because you haven't taken advantage of any of the advantages of C++. I know I used to program the same way which is probably why it frustrates me so much to see so many people coding that way now. Honestly, learn to use the SC++L properly. Don't use FILE *, use std::ifstream and std::ofstream. Don't use raw pointers, use either smart pointers or avoid using pointers entirely (a lot of pointer uses are unnecessary if you modify your designs slightly).

Answer me this. How much coding have you done in the last month? How many times has one of your programs crashed during that time? How much time have you spent tracking down and fixing those errors?

In the past month I have written a lot of code. Because I use the SC++L and code safely I write code faster than I used to, write code cleaner than I used to and write code safer than I used to. If I exclude some rather unorthodox coding I did in which crashes were expected I can honestly count on the fingers of one hand the number of times one of my programs has crashed in the past month. The two most recent were a static order of initialisation error which took me less than ten minutes to track down and fix and an error interacting with a library which I hadn't used before (I passed a zero-based index when it expected a one-based index). That error took less than five minutes to fix. In all I've probably spent less than thirty minutes fixing bugs which caused crashes over the last month (excluding the unorthodox code). The SC++L can save you a lot of trouble. Learn it. Use it. Love it.

Oh, and you use pos and mainend before you initialise them.

Enigma
I noted that you use delete to deallocate an array, which is incorrect, according to
http://www.parashift.com/c++-faq-lite/freestore-mgmt.html#faq-16.11

You must use "delete[] SomePointer" instead.
-- Top10 Racing Simulation needs more developers!http://www.top10-racing.org
"An example... The loader finds a new modelobject in the file. It creates a new cModelObject* and fills it in with the information it finds in the file (also the SomePointer that really is a pointer to a shared material class).
Then pushes the object onto the objects vector and delete the object pointer so the next time it finds an object it can make a new object and use the same pointer for the next new object."

This is so easy to fix. It does not require smart pointers or anything fancy. It requires nothing more than fixing one line:

std::vector<SomeClass> Vector;SomeClass *A = new SomeClass;A->InitPointer();Vector.push_back(*A);A=0; // you had "delete A;" which is WRONG

This topic is closed to new replies.

Advertisement

Popular Topics

Aybe One
How do you manage yourself and your time as years pass on? GDNet Lounge
ReignOnU
Do your part to stop global warming and climate change GDNet Lounge
Konjointed
C++ how do you use dependency injection? For Beginners
Jikoob
Too many managers? General and Gameplay Programming
aigan
The promise of freedom in story games Game Design and Theory
konev13
Syncing 3D animation with hitboxes General and Gameplay Programming
Advertisement

Recommended Tutorials

diegoarj
Procedural Modeling of Buildings with Shape Grammars Engines and Middleware
Myopic Rhino
Simple STL Logging System General and Gameplay Programming
Myopic Rhino
Asset Management with ionForge Evolution Business and Law
stimarco
Pirates of Penzance Business and Law
Myopic Rhino
Peter Stock Interviews

Reticulating splines

About GameDev.net
Community Guidelines
Terms of Service
Privacy Policy
Contact Us
Copyright (c) 1999-2024 GameDev.net, LLC
Back to Top