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

Destroying singletons can be tricky business, but you could solve it by a simple funciton like "destroy()":

Code:

---

class Visualisation
{
  public:
      ...
      void destroy() { delete _instance; _instance = NULL; };
      ...
}

---

Of course, it would be unwise to destroy the singleton untill you are 100% sure that it's no longer needed, and just to make sure, any other function (assuming the singleton is actually doing something, rather than just "existing") should check for NULL in the instance, just in case someone decides to call destroy in the middle of something.

You could also use smart pointers. Very good alternative.

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:

Quote:

---

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;
}

---

And you are including the Sprite.h in the Visualization file, right?

Not sure that goes on here, it looks right to me.

--
Mats

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;
}

---

Quote:

---

Originally Posted by matsp

Not sure that goes on here, it looks right to me.

---

Except that if spriteData is allocated with new[] (otherwise it could be made BYTE and avoid allocations)
it should be deleted with delete[]

And why not to make it vector?

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;

---

Quote:

---

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

Quote:

---

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;
}

---

Thank you very much for your reply.

The only problem is that its a bit to advanced for me, so Im having a hard time following it.

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;
}

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

        private:
                BYTE *spriteData;
...
};

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

---

This is what visual studio says:

Quote:

---

Detected memory leaks!
Dumping objects ->
.\HapiRendererDirect3D.cpp(443) : {332} normal block at 0x0026A050, 16384 bytes long.
Data: < > F0 FF F8 FF ED FF F6 FF EE FE F6 FF F0 FC F6 FF
{256} normal block at 0x00262DE0, 52 bytes long.
Data: < > 01 00 CD CD 01 00 00 00 00 00 00 00 00 00 00 00
Object dump complete.
The thread 'Win32 Thread' (0x1b5c) has exited with code 0 (0x0).
The program '[6452] HelloWorld.exe: Native' has exited with code 0 (0x0).

---

Quote:

---

friend class Singleton<Visualization>; // so Singleton<T> can access our ctor/dtor.

---

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

Quote:

---

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

---

Why? It works fine right now. Changed to but nothing happened. Im not questioning you here, im just wondering why it has to be static

Quote:

---

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

---

Thanks for the link.

Again, thanks for the code you posted.

You're welcome. :)

Quote:

---

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.

Again, thank you very very much.

Quote:

---

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

---

I though that was the same?

Quote:

---

BTW why does destroy need a return value?

---

I have been told to put error checking on everything that I can. Not that I like it, but i guess I have gotten used to it

Quote:

---

What is in HapiRendererDirect3D.cpp?

---

Its the API im using. It is directX stripped for everything. All i have is a screen pointer so I have to do all the blitting and everything myself

Quote:

---

If you allocate the sprite data with something besides new you should not use delete on it.

---

I know that the sprite data should be deleted "the array way". Thats all it says on the website.

Quote:

---

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.

---

So, in my case it would be:
friend class Sprite; ?

EDIT:
Simply adding the Sprite class as a friend to the Visualisation class did not work. I compiled, but the memory leak was still there.

Thanks again, I really appreciate your effort to help.

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

--
Mats

Quote:

---

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. ;)

Quote:

---

Originally Posted by h3ro

Quote:

---

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.

Quote:

---

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

My guess is that LoadTexture has a related function to "delete" a texture in some way - exactly what it is called should be part of the documentation for the library you are using. This is what you should called to "destroy" the texture - note that there may well be other resources tied to a texture, such as a "texture object" in the graphics library or graphics driver, so there's further reasons to destroy it properly.

--
Mats

Quote:

---

Originally Posted by h3ro

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

---

What is the array method?

delete[]

free()?

Mind giving us the link to the documentation?

Something weird most be happening. I can use the class functions in the Sprite class that I have created before, but if i try to add a new one, i get an error (same as before)

Code:

---

If I add
void Sprite::foo()
{
// Do noting
}

and call it:
spriteList[0]->foo();

I get an error.

---

But if I use one of the older functions, it works fine

Maybe your sprite isn't allocating memory correctly?

Im sorry, but I cant post a link to the documentation, but it says that caller of the loadTexture function has to delete the returned data them self. And that it should be deleted using the array method.

Also, there is no deleteTexture function or something like that.

Quote:

---

Maybe your sprite isn't allocating memory correctly?

---

I think it does, as I can draw to the texture without any problems. Its just deleting it that is hard.

EDIT:
What might be a reason for non of my new functions working? Like the foo() example

Quote:

---

Originally Posted by h3ro

Im sorry, but I cant post a link to the documentation, but it says that caller of the loadTexture function has to delete the returned data them self. And that it should be deleted using the array method.

Also, there is no deleteTexture function or something like that.


I think it does, as I can draw to the texture without any problems. Its just deleting it that is hard.

EDIT:
What might be a reason for non of my new functions working? Like the foo() example

---

You'd have to post a complete, compilable example of what is going wrong - although the likely scenario is that you end up finding it yourself if you try to create this - but it's hard to guess what goes on in "..." that you haven't posted, and I suspect:
1. You are not too keen on posting it.
2. I'm certainly not keen on reading loads of code to find what's wrong.

--
Mats

Code:

---

You'd have to post a complete, compilable example of what is going wrong - although the likely scenario is that you end up finding it yourself if you try to create this - but it's hard to guess what goes on in "..." that you haven't posted, and I suspect:
1. You are not too keen on posting it.
2. I'm certainly not keen on reading loads of code to find what's wrong.

---

I cant post my entire code or the needed libraries needed to compile it due to copy rights. I tried to take as much out as possible, and I can use new functions now, but I still get the memory loss even though I call the destructor for the sprites when the program closes.

Sorry for not posting the needed files, but thanks a lot for the help

I think the correct thing to do is to call "delete [] spritedata" - but I'm not 100% sure.

--
Mats

Reworked the whole thing and removed a pointer that casued me nothing but trouble.

matsp: Your right, its delte[]

Its working now, with no memory leaks.

Thanks a lot to matsp and sethjackson!