Am I Leaking? STL + Texture Manager...

Started by
12 comments, last by GroZZleR 20 years, 4 months ago
Hey all, I've got this texture manager in the works. Right now I'm just getting comfortable with STL, because I've never worked with it before. I'm jus wondering if the following code is leaking memory or not. I use new without delete in a vector, but does the STL Vector clean it up for me? Or do I still have to go in and delete it? Here it is:

#include <stdio.h>
#include <string.h>

#include <vector>
using namespace std;

class CTexture
{
public:
	CTexture(char *szTextureName)
	{
		sprintf(m_szTextureName, szTextureName);
		m_iReferenceCount = 1;
	}

	void IncreaseReferenceCount(void)
	{
		m_iReferenceCount++;
	}

	void DecreaseReferenceCount(void)
	{
		m_iReferenceCount--;
	}

	int GetReferenceCount(void)
	{
		return m_iReferenceCount;
	}

	char *GetTextureName(void)
	{
		return m_szTextureName;
	}

private:
	int m_iReferenceCount;
	char m_szTextureName[255];
};

class CTextureManager
{
public:
	~CTextureManager()
	{
		m_TextureList.clear();
	}

	CTexture *LoadTexture(char *szTextureName)
	{
		// Is the texture already loaded?

		for(vector<CTexture *>::iterator TextureIterator = m_TextureList.begin(); TextureIterator != m_TextureList.end(); TextureIterator++)
		{
			if(strcmp(szTextureName, (*TextureIterator)->GetTextureName()) == 0)
			{
				(*TextureIterator)->IncreaseReferenceCount();
				return (*TextureIterator);
			}
		}

		// Its not loaded, lets press on.

		m_TextureList.push_back(new CTexture(szTextureName));
		return (CTexture *)m_TextureList.back();
	}

	void UnloadTexture(CTexture *pTexture)
	{
		// Find The Texture...

		for(vector<CTexture *>::iterator TextureIterator = m_TextureList.begin(); TextureIterator != m_TextureList.end(); TextureIterator++)
		{
			if(pTexture == (*TextureIterator))
			{
				(*TextureIterator)->DecreaseReferenceCount();
				// If we have 0 or less (less?! WTF!) references to the texture, delete it.

				if((*TextureIterator)->GetReferenceCount() < 1)
					m_TextureList.erase(TextureIterator);

				return;
			}
		}

		return;
	}

	void UnloadTexture(char *szTextureName)
	{
		// Find The Texture...

		for(vector<CTexture *>::iterator TextureIterator = m_TextureList.begin(); TextureIterator != m_TextureList.end(); TextureIterator++)
		{
			if(strcmp(szTextureName, (*TextureIterator)->GetTextureName()) == 0)
			{
				(*TextureIterator)->DecreaseReferenceCount();
				// If we have 0 or less (less?! WTF!) references to the texture, delete it.

				if((*TextureIterator)->GetReferenceCount() < 1)
					m_TextureList.erase(TextureIterator);
				return;
			}
		}

		return;
	}


	void PrintTextureList(void)
	{
		for(vector<CTexture *>::iterator TextureIterator = m_TextureList.begin(); TextureIterator != m_TextureList.end(); TextureIterator++)
			printf("\t%s\n", (*TextureIterator)->GetTextureName());
		return;
	}


	vector<CTexture *> m_TextureList;
};

int main()
{
	CTextureManager Manager;
	CTexture *pTexture1a = Manager.LoadTexture("One");
	CTexture *pTexture1b = Manager.LoadTexture("One");
	CTexture *pTexture1c = Manager.LoadTexture("One");
	CTexture *pTexture2a = Manager.LoadTexture("Two");
	CTexture *pTexture2b = Manager.LoadTexture("Two");

	Manager.PrintTextureList();

	printf("%s\n", pTexture1a->GetTextureName());
	printf("%s\n", pTexture1b->GetTextureName());
	printf("%s\n", pTexture1c->GetTextureName());
	printf("%s\n", pTexture2a->GetTextureName());
	printf("%s\n", pTexture2b->GetTextureName());

	printf("REF COUNT: %d | %d\n", pTexture1a->GetReferenceCount(), pTexture2a->GetReferenceCount());
	Manager.UnloadTexture(pTexture2b);
	printf("REF COUNT: %d | %d\n", pTexture1a->GetReferenceCount(), pTexture2a->GetReferenceCount());

	Manager.PrintTextureList();

	printf("\n");

	Manager.UnloadTexture(pTexture2a);
	Manager.PrintTextureList();

	return 0;
}
It works exactly how I want it to work thusfar, but I just want to make sure I'm not creating a memory leaked mess before I continue with it =) Thanks all! [edited by - GroZZleR on December 15, 2003 5:16:09 PM]
Advertisement
If you call new, you must call delete

The same is true for vectors. The vector just contains a list of pointers, the vector will be responsible for maintaing it's size based on that. When you call vector::clear(), it will erase the pointers but not the memory it points to.

so, to delete the memory, do the following:
for(vector<CTexture *>::iterator it = m_TextureList.begin(); it != m_TextureList.end(); ++it){  assert( (*it) ); //I like to assert because it should be valid  delete (*it); //You must dereference the iterator to get to the memory it points to}m_TextureList.clear();  //To clear the pointers from the list


[edited by - pjcast on December 15, 2003 5:28:35 PM]
When you do a m_TextureList.clear() it will delete the pointers, but the actual data you assigned with new will still be somewhere in memory, undeleted. With a garbage collected language, lost memory like that would automatically get cleared but in c++ you have to keep track of everything and delete it yourself

What pjcast said
quote:Original post by pjcast
assert( (*it) ); //I like to assert because it should be valid

you can call delete on a NULL pointer, so this is pointless.
--- krez ([email="krez_AT_optonline_DOT_net"]krez_AT_optonline_DOT_net[/email])
quote:Original post by krez
quote:Original post by pjcast
assert( (*it) ); //I like to assert because it should be valid

you can call delete on a NULL pointer, so this is pointless.


I know, but what the point I was trying to say with that comment is, that the program should let me know that the pointer is null. Now, if i just said:

if( (*it) )
delete (*it);

then I wouldn''t be made aware of the error. Each item in the vector should not be a null pointer ever. When you add a value, it should in this case point to memory, and once you delete that item you either delete, then remove that entry. Or you delete all, and clear all entries; therefore, I prefer to use assert (though I could use exception) for debugging.
even if the pointer was NULL (0), you could still delete it (you don''t need the "if" much less the "assert"). if it wasn''t NULL, but pointed to an invalid address, neither would help anyway.

i''d check that the pointer wasn''t NULL or invalid before putting it in the vector.

eh, don''t mind me, i was just pointing it out in case someone else reading didn''t know. i guess it sounded like i was trying to press my style on you. i hope i didn''t come off as a dick.
--- krez ([email="krez_AT_optonline_DOT_net"]krez_AT_optonline_DOT_net[/email])
Alright, thanks for the tips and I''m now clearer on the STL portion and such.

However, I now have this lingering question.

	void UnloadTexture(CTexture *pTexture)	{		// Find The Texture...		for(vector<CTexture *>::iterator TextureIterator = m_TextureList.begin(); TextureIterator != m_TextureList.end(); TextureIterator++)		{			if(pTexture == (*TextureIterator))			{				(*TextureIterator)->DecreaseReferenceCount();				// If we have 0 or less (less?! WTF!) references to the texture, delete it.				if((*TextureIterator)->GetReferenceCount() < 1)					m_TextureList.erase(TextureIterator);				return;			}		}		return;	}


Mainly this section:
if((*TextureIterator)->GetReferenceCount() < 1)					m_TextureList.erase(TextureIterator);


How do I: delete TextureIterator and then use it to erase it from the list at the same time? =P

If I erase it first, then delete it, will it still be pointing at valid data that can be deleted?
iterators need not be deleted or erased from the list.

To remove a entry:

delete memory iterator points to, like this

delete (*iterator)

then remove the empty pointer from the list

m_TextureList.erase( iterator )

remember, you did not delete the iterator (because you never allocated it), you only deleted what it pointed to.



[edited by - pjcast on December 16, 2003 3:58:30 AM]
Correct me if I am wrong, but I believe erasing from a vector invalidates the iterator. This may be only lists, I don't have the docs handy ATM.

So, you should do the following:

TextureIterator = m_TextureList.erase(TextureIterator);

instead of just

m_TextureList.erase(TextureIterator);

because the erase function returns an iterator to the next item.

daveandrews.org - a Christian Programmer's Weblog | Dusty Engine - a task engine using Irrlicht | Computer Guys Run the World
How to be a Programmer

[edited by - Ronin Magus on December 17, 2003 5:32:56 PM]
I thought that if you have a vector in a class which is in static memory (ex: int x) then when that variable exists the scope the vector inside it will completely erase itself as well automatically. Don''t even need a deconstructor... Am I wrong here?

This topic is closed to new replies.

Advertisement