Archived

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

Am I Leaking? STL + Texture Manager...

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

If you intended to correct an error in the post then please contact us.

Recommended Posts

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]

Share this post


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

Share this post


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

Share this post


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

Share this post


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

Share this post


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

Share this post


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

Share this post


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

Share this post


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

Share this post


Link to post
Share on other sites
quote:
Original post by Enokh
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?


Do you mean that if you have a vector inside an object, and the object goes out of scope (destructs) then the vector will erase itself? If that''s what you meant, then yes.. the vector erases itself in its destructor.


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

Share this post


Link to post
Share on other sites
quote:
Original post by krez
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.


You completely did not read, and/or comprehend, what he just said.

Yes, you CAN call delete on something that is NULL without issue, but, it should not ever be null, and if it is, he would like to know about it (error checking). If you just call delete without any type of checking, there is no way to know when something goes bad.

Share this post


Link to post
Share on other sites
for managers (mesh or texture) i use an std::map, and smart pointers instead of pointers. So when i use the clear() method, smartpointer destructor takes care of memory deletion.
I think its a kinda good use of SPs. Opinions anyone?

Share this post


Link to post
Share on other sites