Trying to solve my memory leaks

This is a discussion on Trying to solve my memory leaks within the C++ Programming forums, part of the General Programming Boards category; Hallo, I am kind of new to taking care of the memory I assign. Before now, I have mostly ignored ...

  1. #1
    Registered User
    Join Date
    Oct 2006
    Location
    UK/Norway
    Posts
    485

    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

  2. #2
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    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.
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  3. #3
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,188
    You could also use smart pointers. Very good alternative.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.
    For information on how to enable C++11 on your compiler, look here.
    よく聞くがいい!私は天才だからね! ^_^

  4. #4
    Registered User
    Join Date
    Oct 2006
    Location
    UK/Norway
    Posts
    485
    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;
    }

  5. #5
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    And you are including the Sprite.h in the Visualization file, right?

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

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  6. #6
    Registered User
    Join Date
    Oct 2006
    Location
    UK/Norway
    Posts
    485
    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;
    }

  7. #7
    CSharpener vart's Avatar
    Join Date
    Oct 2006
    Location
    Rishon LeZion, Israel
    Posts
    6,424
    Quote Originally Posted by matsp View Post
    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?
    The first 90% of a project takes 90% of the time,
    the last 10% takes the other 90% of the time.

  8. #8
    Registered User
    Join Date
    Oct 2006
    Location
    UK/Norway
    Posts
    485
    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

  9. #9
    Registered User
    Join Date
    Aug 2005
    Posts
    96
    Quote Originally Posted by h3ro View Post
    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;
    }

  10. #10
    Registered User
    Join Date
    Oct 2006
    Location
    UK/Norway
    Posts
    485
    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:
    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).
    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?

    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

    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.
    Last edited by h3ro; 02-18-2008 at 02:13 PM.

  11. #11
    Registered User
    Join Date
    Aug 2005
    Posts
    96
    You're welcome.

    Quote Originally Posted by h3ro View Post
    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.

  12. #12
    Registered User
    Join Date
    Oct 2006
    Location
    UK/Norway
    Posts
    485
    Again, thank you very very much.

    You should also return true or false for bool values, not 1 or 0.
    I though that was the same?

    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

    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

    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.

    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.
    Last edited by h3ro; 02-18-2008 at 02:45 PM.

  13. #13
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Yes, where did "LoadTexture" get the memory to store the sprite data?

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  14. #14
    Registered User
    Join Date
    Aug 2005
    Posts
    96
    Quote Originally Posted by h3ro View Post
    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 View Post
    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.

  15. #15
    Registered User
    Join Date
    Oct 2006
    Location
    UK/Norway
    Posts
    485
    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

Page 1 of 2 12 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Assignment Operator, Memory and Scope
    By SevenThunders in forum C++ Programming
    Replies: 47
    Last Post: 03-31-2008, 06:22 AM
  2. Memory Leaks?
    By jimmymax in forum C++ Programming
    Replies: 3
    Last Post: 11-23-2007, 02:57 AM
  3. Question about Memory Leaks with *char
    By Diamonds in forum C++ Programming
    Replies: 11
    Last Post: 12-16-2002, 06:00 AM
  4. Is it necessary to write a specific memory manager ?
    By Morglum in forum Game Programming
    Replies: 18
    Last Post: 07-01-2002, 01:41 PM
  5. Memory leaks
    By Eber Kain in forum C++ Programming
    Replies: 3
    Last Post: 12-20-2001, 11:58 AM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21