[Solved] Map and Memory Leak

[zeid]

Ok, so for this game engine I have been working on I use a series of managers to hold things such as models, textures, etc.

Any way these manager classes are pretty much singletons with a map containing a name and a pointer to the location of each entity that the manager is dealing with.

Code: Select all

typedef std::map<std::string,Entity*> EntityMap; //the map's attribute types
EntityMap m_entities;

It is all fine and dandy, up to the point where I'm ending the program. Because of the fact that everytime I add an entities reference to this map I am adding another pointer that needs deleting I need a way to cycle through and delete all these pointers so that I don't get a memory leak. I have attempted a few ways of doing this, most resulting in my program ending unpleasantly with an "Assertion Error", and the rest not actually fixing the problem.

Heres one of my attempts, I had alot more but I think it would be pointless showing more of what doesn't work

Code: Select all

for(i = m_entities.end(); i != m_entities.begin(); --i)
{
	delete i->second;
	i = m_entities.erase(i);
}

So Is anyone familiar enough with maps and pointers to help me with this problem?

P.S. another idea I had was a vector of pointers that point to the pointers. :P But I thought I would check here first to see if there was a better way then creating a whole other list of information that could be unnecassary.

[andrew]

Is this right about the erase member?

The return value is the element after the last element erased.

I got the information here.

Would it mean that the code won't work because it's going the wrong direction?

And would this fix it?

Code: Select all

for(i = m_entities.begin(); i != m_entities.end(); ++i)
{
   delete i->second;
   i = m_entities.erase(i);
}

Hopefully it helps to fix it.

[Bakkon]

I'm also using maps in my current project to store textures. Here's what my cleanup function looks like. If someone sees a problem with my solution, I'll take some critiques.

Code: Select all

void Texture::UnloadTextures()
{
	for(map<string,Texture*>::iterator Iter = TextureList.begin(); Iter != TextureList.end(); ++Iter)
	{
		printf("Deleting texture: %s\n", (*Iter).first.c_str());
		delete (*Iter).second;
	}
	
	TextureList.clear();
}

[K-Bal]

try this

Code: Select all

for(i = m_entities.end(); i != m_entities.begin();)
{
	delete i->second;
	i = m_entities.erase(i);
}

[andrew]

Thanks K-Bal for pointing out that incrementing or decrementing an iterator while using the erase member will not work.
Bakkon's code also looks like a good way to do it.

Here is some code that shows how it works with vectors:

Code: Select all

vector <int> m_entities;
vector <int>::iterator i;

m_entities.push_back(1);
m_entities.push_back(2);
m_entities.push_back(3);
m_entities.push_back(4);

for(i = m_entities.begin(); i != m_entities.end();)
{
    printf("%i \n", *i);
    i = m_entities.erase(i);
}

if (m_entities.empty())
{
    printf("m_entities is empty\n");
}

Here is the output:

Code: Select all

C:\Documents and Settings\Andrew\Desktop\code>vectortest
1
2
3
4
m_entities is empty

This code does not work:

Code: Select all

for(i = m_entities.end(); i != m_entities.begin();)
{
    printf("%i \n", *i);
    i = m_entities.erase(i);
}

What happens is when the iterator is set to the end, it actually points to an element that is one beyond the last element. So when erase is called it is erasing something that doesn't belong to the application. And I'm sure something really bad is happening when this is done. Who knows which memory location is being erased?

Here is the output:

Code: Select all

C:\Documents and Settings\Andrew\Desktop\code>vectortest
196633

[dandymcgee]

Yeah dude, following up on Bakkon's code I would highly recommend looking up iterators.

[zeid]

Here's what I came up with from the advice you guys gave.

Code: Select all

	for(i = m_models.begin(); i != m_models.end(); i++)
	{
		delete i->second;
		MessageBox(NULL, "Model destroyed" , "Debug", 0);
	}

It seems that I was overdoing it with the m_models.erase(i).

Thanks everyone for the help, my engine is now memory leak free.

[K-Bal]

Here's what I came up with from the advice you guys gave.

Code: Select all

	for(i = m_models.begin(); i != m_models.end(); i++)
	{
		delete i->second;
		MessageBox(NULL, "Model destroyed" , "Debug", 0);
	}

It seems that I was overdoing it with the m_models.erase(i).

Thanks everyone for the help, my engine is now memory leak free.

This causes a new leak. You now have a bunch of dead pointers in your map.

[zeid]

Well I would use the code you provided, but it throws the assertion error that I mentioned.
I've been using the visual leak detector (and reccomend it to others) it confirms to me that I have no memory leaks.
http://www.codeproject.com/KB/applicati ... ector.aspx
Is VLD wrong?

[MarauderIIC]

Here's what I came up with from the advice you guys gave.
It seems that I was overdoing it with the m_models.erase(i).

You can get the same result by clearing your map after you've deleted everything.

This causes a new leak. You now have a bunch of dead pointers in your map.

This isn't a leak, because his memory has been deallocated. It's merely unsafe :)
Simple to fix, though.

Code: Select all

	for(i = m_models.begin(); i != m_models.end(); i++)
	{
		delete i->second;
		i->second = NULL;
		MessageBox(NULL, "Model destroyed" , "Debug", 0);
	}

Unless, of course, you want to just do...

Code: Select all

	for(i = m_models.begin(); i != m_models.end(); i++)
	{
		delete i->second;
		MessageBox(NULL, "Model destroyed" , "Debug", 0);
	}
	m_models.clear();

But if it's cleanup, it "doesn't really matter" if you set the pointers to NULL or clear your map, nor if you clear it.

If you really want to erase and delete at the same time,

Code: Select all

for (i = m_models.begin(); i != m_models.end(); i = m_models.begin()) {
        delete i->second;
        //I would probably do a i->second = NULL; for safety, in case the code is modified later.
        m_models.erase(i);
    }