Archived

This topic is now archived and is closed to further replies.

difficulty deleting pointers from std::vector

This topic is 5095 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

Recommended Posts

No matter what I do I cannot delete pointers from std::vectors. In this case, my std::vectors hold VWorldObject pointers. I am going through each leaf in my BSP destructor and trying to delete the entities from the appropriate std::vector if there are any.
	for(int i = 0; i < mNumLeafs; i++)
{
if(mpLeafs[i].entitylist.size() > 0)
{
std::for_each(mpLeafs[i].entitylist.begin(),mpLeafs[i].entitylist.end(),Deleter<VWorldObject>());
mpLeafs[i].entitylist.clear();
}

}

here is the code for the Deleter structure:
template<class T>
struct Deleter
{
void operator()(T* obj)
{
if(obj)
{
delete obj;
//	obj	=	NULL;

}
}
};

When I step through the code, it gives me an access violation in the destructor of VWorldObject. Specifically on this line when delete[] is called:
	if(mpBrushes)
{
delete[]	mpBrushes;
mpBrushes	=	NULL;
mnumOfBrushes = 0;
}

The weird thing is the this pointer for the VWorldObject class is valid, so Im not sure why I am getting so many problems. EDIT: Did I even post enough information? I'm not sure if I'm being vague or not. [edited by - Shadow12345 on February 7, 2004 11:27:58 AM] [edited by - Shadow12345 on February 7, 2004 11:35:03 AM] [edited by - Shadow12345 on February 7, 2004 11:42:52 AM]

Share on other sites
Im gonna think that you want Deleter< VWorldObject* >() instead of Deleter< VWorldObject >(). Im making the assumption that you have VWorldObject pointers, otherwise you would not need to be deleting them, because they would automatically be deleted when the container object was deleted.

[EDIT]
Stupid HTML >_<

[edited by - porthios on February 7, 2004 11:38:13 AM]

Share on other sites
This is where it gets pretty complicated, but doing what you said doesn''t work, it gives errors (I''ve tried that before). Sorry about the HTML thing.

I am really stumped with this problem, I have *absolutely* no clue what is wrong. On the other hand, I''ve got a huge project, and I guess the problem could potentially be in a lot of different places. If anybody wants me to post more code I will, but I don''t want to post too much.

Share on other sites
The reason porthois' suggestion didn't work is because Delete is templated on class T and its operator() takes a parameter of type T*, so templating it on VWorldObject makes it delete VWorldObject*'s while templating it on VWorldObject* would make it delete VWorldObject**'s.

I don't think you've posted enough code for us to see where the problem is. Usually errors like this mean that either a) the WorldObject pointer is not valid or b) mpBrushes is not valid. Can you post the any code from VWorldObject that does anything with mpBrushes, including all constructors and assignment operators, and also the code where you populate your vector?

Enigma

EDIT: typo

[edited by - Enigma on February 7, 2004 11:56:41 AM]

Share on other sites
When I create a VWorldObject I set mpBrushes equal to NULL.
Here's the tEntBrush class and the tEntPlane classes that I am using.

struct	tEntPlane{	tEntPlane()	{		d = 0;		vNormal = Vector3(0,0,0);	}	float	d;	Vector3	vNormal;};struct	tEntBrush{	tEntBrush()	{		numPlanes = 0;		mpPlanes	=	NULL;		textureID	=	0;	}	~tEntBrush()	{		if(mpPlanes)		{			delete[] mpPlanes;			mpPlanes = NULL;		}		numPlanes = 0;		textureID = 0;	}		int			numPlanes;	tEntPlane	*mpPlanes;	int			textureID; //Index into BSP's textures};

I didn't know whether or not to put constructors/destructors in those structs, but I figured if I did I was just being safe. I'll probably edit and keep adding more code to this post.

EDIT:
I don't know what to post, so here is the full VWorldObject constructor/destructor. If this is TOO much I'll gladly take it down.

VWorldObject::VWorldObject(){	this->mvStates = SOLID_ENT;// + ACTIVE_ENT; //Always solid by default	this->mMaxs = Vector3(0, 0, 0);	this->mMins = Vector3(0, 0, 0);	this->mPosition = Vector3(0, 0, 0);	this->mnumOfBrushes = 0;	this->mnumOfFaces = 0;	this->mfaceIndex = 0;	this->mbrushIndex = 0;	this->mpBrushes	=	NULL;	this->mDisplacement = Vector3(0,0,0);	this->mFrame = 0;	this->mFrameTouch = 0;	this->mpcTeamName	=	NULL;	this->mpNeighbor	=	NULL;	this->mVelocity = Velocity();	this->mTimeInAir = 0;	this->mFallVelocity.Dir = Vector3(0,-1,0);	this->mFallVelocity.Mag = 0;	this->mpModel	=	NULL;	mLeafs.clear();	meCollisionType	=	eBrush; //default }VWorldObject::~VWorldObject(){	if(mpBrushes)	{		delete[]	mpBrushes;		mpBrushes	=	NULL;		mnumOfBrushes = 0;	}	if(mpcTeamName)	{		delete[]	mpcTeamName;		mpcTeamName	=	NULL;	}	if(mpModel)	{		delete	mpModel;		mpModel	=	NULL;	}	mLeafs.clear();}

One thing I legitimately am not sure about is how to deal with the mLeafs vector. It is an std::vector, and I am not sure if I should init it by doing mLeafs.clear() as I am doing above, or init it by doing mLeafs = std::vector(); They both seem to work. Keep in mind my program works perfectly with what I have, it is only when I try to delete my entities that I get errors (I can delete the BSP without deleting the entities and it works fine, but it takes up too much memory when I do that)

[edited by - Shadow12345 on February 7, 2004 12:07:49 PM]

[edited by - Shadow12345 on February 7, 2004 12:09:26 PM]

[edited by - Shadow12345 on February 7, 2004 12:10:09 PM]

[edited by - Shadow12345 on February 7, 2004 12:10:27 PM]

[edited by - Shadow12345 on February 7, 2004 12:10:49 PM]

Share on other sites
It sounds like that at some point you''re doing a shallow copy of the std::vector that contains your VWorldObject pointers. What does the class that holds that vector look like? Specifically the copy constructor or assignment operator.

Share on other sites
Shallow copies of the vector wouldn't cause this problem, because you'd have multiple pointers to the same object and when it was deleted the first time the member pointers would be set to null. Unless I'm misunderstanding what you're saying. EDIT: D'uh, wasn't thinking clearly, if the memory was reused between deletes then this problem might occur.

Do you ever copy VWorldObject objects (not pointers)? If you don't have a proper copy constructor and assignment operator then this will result in your program attempting to delete the same memory multiple times.

Oh, and while I'm at it, calls like this:
tEntPlane(){	d = 0;	vNormal = Vector3(0,0,0);}

would be better as:
tEntPlane()	:	vNormal(0, 0, 0){	d = 0;}

which can avoid a temporary.

Enigma

[edited by - Enigma on February 7, 2004 12:35:43 PM]

Share on other sites
I think you're double-deleting memory. Try the following code for your Deleter:
template<class T>struct Deleter{    	void operator()(T * & obj) 	{    		if(obj)		{			delete obj;    			obj = NULL;		}	}};

Note that "* &" in the function definition, which is a reference to a pointer (not a pointer to a reference). That way, you can do the "obj = NULL;" and have it actually NULL out the pointer so you don't re-delete the same memory.[/source]

[edited by - aprosenf on February 7, 2004 12:29:46 PM]

Share on other sites
I don't have a copy constructor or assignment operator for the class implemented.

Here is how I'm initializing the VWorldObjects. They are entities stored in a quake3 entity string.

I've got functions to parse each of the types of entities. Here is one that parses a func_door, which derives from VWorldObject

void	BSP::ParseFuncDoor(char	*temp, std::vector<VWorldObject*>	*a){//Allocate memory for a func_door 		Door	*door = new Door; /* --a ton of code taken out here*/		LinkEntityToWorld(door); //adds pointer to bsp leaf entitylist std::vectors 		a->push_back(door);		 //adds to GameExport std::vector 	}

Note that I only try deleting the objects themselves in the BSP constructor. All entities in the GameExport std::vector are manually removed from the vector using std::vector::erase and iterators.
EDIT:
I tried what you suggested Aprosenf and it does not work for some reason. Why wouldn't the pointer be set to null in the original version? I suspect that for some reason the pointer isn't being set to null but I don't know why it isn't.

EDIT1:
quote:

Do you ever copy VWorldObject objects (not pointers)? If you don't have a proper copy constructor and assignment operator then this will result in your program attempting to delete the same memory multiple times.

No, I never attempt to copy VWorldObject objects. they are all pointers. The only possible exception would be the Player, but that too is also a pointer and also derives from VWorldObject.
[edited by - Shadow12345 on February 7, 2004 12:31:37 PM]

[edited by - Shadow12345 on February 7, 2004 12:33:46 PM]

[edited by - Shadow12345 on February 7, 2004 12:35:31 PM]

Share on other sites
Well, if your program gets an access violation during a delete statement, then 99.9% of the time you''re either trying to delete NULL memory (which you''re clearly not), you''re trying to delete an uninitialized pointer (which you''re also not), or you''re trying to delete memory that''s already been deleted. Make sure you also check the destructors of subobjects since calling delete [] objs; calls the destructors of objs before it trys to free the memory. The reason I think you''re deleting the same memory twice is because your original Deleter struct deleted the memory but didn''t NULL-out the pointer, which would cause that pointer to be deleted a second time at some later point in its life. However, you can''t simply just say obj = NULL in that case, because the pointer is passed by value. You could NULL out that pointer, but that is a copy of the original pointer, and the original pointer wouldn''t by NULL''ed out. That is why I added the "* &" so that NULLing out the pointer reference would NULL out the original. If doing those changes didn''t work, then I''m really baffled as to what''s happening.

Share on other sites
OK, to try and find out whether this is a) a multiple deletion problem or b) a not-really-a-VWorldObject deletion problem, can you add a global:
std::vector<int> vWorldObjectValidator1;std::deque<int> vWorldObjectValidator2;

and in the VWorldObject constructor add:
vWorldObjectValidator1.push_back((int)this);vWorldObjectValidator2.push_back((int)this);

and at the start of the VWorldObject destructor add:
if (std::find(vWorldObjectValidator1.begin(), vWorldObjectValidator1.end(), (int)this) == vWorldObjectValidator1.end()){	// some error reporting, i.e. message box or std::cerr << .. reporting invalid object	std::exit(-1);}else if (std::find(vWorldObjectValidator2.begin(), vWorldObjectValidator2.end(), (int)this) == vWorldObjectValidator2.end()){	// some error reporting, i.e. message box or std::cerr << .. reporting already deleted object	std::exit(-1);}vWorldObjectValidator2.erase(std::find(vWorldObjectValidator2.begin(), vWorldObjectValidator2.end(), (int)this));

Enigma

Share on other sites
To be perfectly honest I am still totally befuddled as to what the problem is. I have tried all of your suggestions but I was still generating the exact same error somehow. I have finally got it so that it works, however I do not know what was wrong to be honest. I'm deleting them using std::for_each in my GameExport structure before I delete my BSP, then when I delete the BSP I just clear the raw pointers in the leaf std::vector by calling clear() on them. The funny thing is I had been doing this for a long time and it was not working, but I must've changed something somewhere to get it working.

You can obviously tell I still really have no clue what is going on...subsequently I think I am going to look into boost::shared_ptr, although boost.org seem to be down right now.

However, you guys have been really really great. I wish everybody was as patient and thoughtful as you guys. You certainly kept pointing me in the right direction and I think I have a better idea of what might be wrong than when I started.

[edited by - Shadow12345 on February 7, 2004 2:50:43 PM]

[edited by - Shadow12345 on February 7, 2004 2:51:53 PM]