Thread: Trying to solve my memory leaks

Hallo,

I am kind of new to taking care of the memory I assign. Before now, I have mostly ignored it (clever right?)

Right now im having a huge problem trying to get rid of a memory leak i have, so if anyone could take a look I would be a very happy man.

Code:

// My class
class Visualisation
{
	public:
		static Visualisation &GetInstance();
...

	protected: 
		Visualisation();
		~Visualisation(void);
		

	private:
		static Visualisation *_instance;
...

};

#define VIZ Visualisation::GetInstance()



Visualisation & Visualisation::GetInstance()
{
	if (_instance == NULL)
		_instance = new Visualisation;

	return *_instance;
}

Visualisation::~Visualisation(void)
{
	delete _instance;
}

I think the problem has to be in that part of the code(I know for sure it is in the Visualisation class) as its the only new I have in my code. I dont call the destructor, as I have been told that the program calls it when it ends (also tried to call it myself just to be sure, no difference)

Regards,
Ole Kristian

Thanks for your help. It worked well. I have now started to get remove some more memory leaks.

In my Visualisation class I load sprites and save them in a vector, and when I destroy the Vis class I want all the sprites to be removed as well. But I get this error:

Visualisation.obj : error LNK2001: unresolved external symbol "public: void __thiscall Sprite::destroySprite(void)" (?destroySprite@Sprite@@QAEXXZ)
Debug/HelloWorld.exe : fatal error LNK1120: 1 unresolved externals

Code:

class Visualisation
{
	public:
		static Visualisation &GetInstance();
		bool destroy();
...

	protected: 
		Visualisation();
		~Visualisation(void);
...

	private:
		static Visualisation *_instance;
		std::vector <Sprite*> spriteList;
...
};


bool Visualisation::destroy()
{
	if (_instance != NULL)
	{
		// Destroy all the sprites
		for(int i = 0; i < spriteList.size(); i++)
			spriteList[i]->destroySprite();    // <--- Problem

		// Destroy the class instance
		delete _instance; 
		_instance = NULL;

		return 1;
	}
	else
		return 0;
}

// The Sprite class
class Sprite
{
	public:
		Sprite();
		~Sprite(void);
		bool loadSprite(std::string fileName);
		void destroySprite();
...

	private:
		BYTE *spriteData;
...
};

void Sprite::destroySprite()
{
	delete spriteData;
}

Yes. I can use all the other functions of the Sprite class.

For example, this works:

Code:

bool Visualisation::destroy()
{
	if (_instance != NULL)
	{
		// Destroy all the sprites
		for(int i = 0; i < spriteList.size(); i++)
			spriteList[i]->loadSprite("SomeSprite.jpg");    // <--- Problem

		// Destroy the class instance
		delete _instance; 
		_instance = NULL;

		return 1;
	}
	else
		return 0;
}

Thanks a lot for your replies, but I still have the same problem, changing it to delete[] did not solve it.

The memory is allocated in this function. I dont have access to the actual LoadTexture function, but I have been told to pass a BYTE pointer to it.

Code:

bool Sprite::loadSprite(std::string fileName)
{
	// Load a texture, if not able to load texture, return 0
	if(!HAPI->LoadTexture(fileName, &spriteData, &width, &height))
		return 0;

	// If the whole thing loaded as it should, return 1
	return 1;
}

// Private class variable
BYTE *spriteData;

And why not to make it vector?

Make what a vector? The spriteList is the only list I have, and it is already a vector

Originally Posted by h3ro

I dont call the destructor, as I have been told that the program calls it when it ends (also tried to call it myself just to be sure, no difference)

Regards,
Ole Kristian

Destructors can be called different ways.....

If you allocate your object on the heap with new, you must delete them yourself (unless you use smart pointers). If you allocate them on the stack, the compiler will destroy your object when your object goes out of scope.

Like so:

Code:

void foo()
{
    MyObject* obj = new MyObject();

    obj->Foo();

    // you are responsible for deleting your object

    delete obj;
}

Code:

void foo()
{
    MyObject obj;

    obj.Foo();

    // compiler destroys the object at the end of the scope
}

You say you tried to call the destructor? How might I ask? The dtor is protected so you would have to call it from a derived class, but you shouldn't do that because your Visualization class is a singleton.

You should never call an object's dtor explicitly (there are some small exceptions though...).

See here:

http://www.parashift.com/c++-faq-lite/dtors.html



The destroy() function approach works, BUT it needs to be a static function.

Also destroySprite() is unnecessary the std::vector will call the Sprite destructor for you.
Delete the sprite data in the sprite destructor. That is what destructors are for, object cleanup.


So here is what I would do.

Code:

template<typename T>
class Singleton
{
private:
    
    static T* instance = 0;
    
private:
    
    Singleton()
    {
    
    }
    
    ~Singleton()
    {
    
    }
    
public:
    
    static T& GetInstance()
    {
        // this is not thread safe AFAIK
        
        if (instance == 0)
            instance = new T();
        
        return *instance;
    }
    
    static void Free()
    {
        delete instance; // deleting null pointers is legal ;)
    }
};

class Visualization : public Singleton<Visualization>
{
private:
    
    friend class Singleton<Visualization>; // so Singleton<T> can access our ctor/dtor.
    
    std::vector<Sprite*> spriteList;
    
private:
    
    Visualization();
    
    ~Visualization();
    
public:
    
    void Foo();
    
    void Bar();
};

Visualization::Visualization()
{
    // do your sprite list init stuff
}

Visualization::~Visualization()
{
    // don't need to kill spriteList
    // our spriteList goes out of scope here
    // and since it is a stack based object
    // the compiler takes care of it for us
}

Sprite::Sprite()
{

}

Sprite::~Sprite()
{
    delete spriteData;
}

You're welcome.

Originally Posted by h3ro

Here is my current code, it compiles, but I will have a memory leak.

Code:

class Visualisation
{
	public:
		static Visualisation &GetInstance();
		bool destroy();
...

	protected: 
		Visualisation();
		~Visualisation(void);
...

	private:
		static Visualisation *_instance;
		std::vector <Sprite*> spriteList;
...
};


bool Visualisation::destroy()
{
	if (_instance != NULL)
	{
		// Destroy the class instance
		delete _instance; 
		_instance = NULL;

		return 1;
	}
	else
		return 0;
}

Does that mean that in my program, the vector list does not have access to the ctor/dtor?

No. The vector doesn't and can't (unless you change the std::vector class) have access
to the ctor/dtor.

Let me see if I can explain better. When we say:

Code:

friend class Singleton<Visualization>;

We are giving the Singleton class access to our Visualization class ctor/dtor.
Now why do we need to do this?
Because (in my example) the Singleton creates the Visualization class, but because the Visualization class ctor/dtor are private nothing can access them.
Friend however allows the Singleton class the needed access.

You should also return true or false for bool values, not 1 or 0.
BTW why does destroy need a return value?

Code:

        if (_instance != NULL)
	{
		// Destroy the class instance
		delete _instance; 
		_instance = NULL;

		return true;
	}
	else
		return false;

See this also:

http://www.parashift.com/c++-faq-lit....html#faq-16.8

Ok now as to the Visual Studio error.

What is in HapiRendererDirect3D.cpp?

Visualization class?
Sprite class?


Another thing I just thought of.
If you allocate the sprite data with something besides new you should not use delete on it.

Originally Posted by h3ro

So, in my case it would be:

Code:

friend class Sprite; ?

No Sprite has a public constructor, you don't need friend.

See here:

http://www.parashift.com/c++-faq-lite/friends.html

You can ignore friend if you don't use the code I provided.

Originally Posted by h3ro

You should also return true or false for bool values, not 1 or 0.

I though that was the same?

Because in C++ bools aren't glorified ints.

Yes, where did "LoadTexture" get the memory to store the sprite data?

LoadTexture is in a library I dont have access to. All i know is that is take a BYTE pointer and that it should again be deleted with the array method