Sign in to follow this  
Marty666

PROBLEM: std::vector and pointers

Recommended Posts

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

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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. :)

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
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.

Share this post


Link to post
Share on other sites
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;
}


Share this post


Link to post
Share on other sites
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

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
"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



Share this post


Link to post
Share on other sites
Quote:
Original post by Glak
std::vector<SomeClass> Vector;
SomeClass *A = new SomeClass;
A->InitPointer();
Vector.push_back(*A);
A=0; // you had "delete A;" which is WRONG

Err, that's a memory leak. You do need to delete A. And I would contend that any initialisation you need to do in InitPointer should be done in the constructor. What's the point of having an object if it might not be valid?

Enigma

Share this post


Link to post
Share on other sites
Quote:
Original post by Marty666
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
For shared resources both smart pointers or implementing a resource manager would both solve the issue you've described. Both solve the problem of when to delete shared data by use of some sort of reference counting mechanism and if that's all you need then smart pointers would probably be the simplest solution. However, resource managers can give you some added control over both creation and deletion, for example, just because you delete the object that contains the last reference to a given resource doesn't mean you have to delete that resource from memory at that time, it might be a commonly used (i.e, it's likely it may need to be used again in the near future) or you might want to delay deletion to a later time when you actually need the memory. However, you should probably stick with smart pointers until you've done some reading up on resource managers...

Mage

Share this post


Link to post
Share on other sites
Hi all...

I'm just a hobby programmer. Everything i learned is from the web and a book called c++ in 24 hours. I spend a week programming a couple of hours a day now. Before that it has been a couple of months doing other stuff...
Anyways, I'd like to become a better programmer so I will certainly look up the SC++L stuff and try to learn. Smartpointers are something new to me, so i'll look that up aswell.

Glak: You're right when you say it will fix the problem, but as enigma says it would be a leak, since I only want the objects to be in the vector after loading. All these pointers were temporary.

johdex: thanx, i'll keep that in mind.

I guess I've got a lot of reading to do now :D

Thanx for all the fast replies ppl!
Marty

Share this post


Link to post
Share on other sites
ok first of all I didn't realize that you are pushing back *A, I thought that you were pushing back A, meaning that your vector holds pointers. I thought that you were handing off ownership. So now I'm trying to figure out why there is even a pointer at all.


//SomeClass *A = new SomeClass;
//A->InitPointer();
Vector.push_back(A());//the code of InitPointer is now in the constructor
//delete A;

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this