Memory Leak inside Static Library

[himitsujanai]

Hi guys I'm new here and I'm very thankful because this forum helps me a lot to answer some question in mind when it comes c++ codes.

I need your suggestions on my leak problem in the game that i'm creating. I already found cause of the leak.. it is the vector of pointers which globally declare inside the library(static library).

//******************** Simple Pseudo Code of the game **********************
//Inside the library
//It was extern in Button.h
extern vector<Button*>ButtonList

//and it was used inside the SetupButton member functon of the Button class
Class Button
{
void SetupButton()
}

//And it was declare at the Button.cpp as
vector<Button*>ButtonList

//It was use inside the SetupButton member function so that all available buttons in the game will be added in the list
void Button::SetupButton()
{
... //setup the property of the button
...//setup the position
...//etc.

//then add to the list
ButtonList.push_back( this );

}

And in the game...
#include "Button.h"
Game::Game()
{
Button btnStartButton, btnExitButton;
btnStartButton.SetupButton();
btnExitButton.SetupExitButton();
}

Game::~Game()
{
ButtonList.clear();

... Delete some objects used
}

Game* pGame
void Main()
{
pGame = new Game()
...
...
pGame->RunGame();

delete pGame;
return;
}

Basically this how the setup made .. the game is running and its ok except there is a reported leak.

I'm wondering if what is the best approach to fix the problem. I cant use delete on every elements because they are not dynamically allocated. One solution I made and it will definitely solved the leak is to make the ButtonList vector as member of a class but the changes in the game and in the library will be huge ... Im asking you guys if you have better idea that can save time and lines of codes.

Im thinking if i can use a singleton class as a based class of the button and put the ButtonList vector as static member of this class but im not so familliar with singleton class so i dont know if its possible


Any inputs will be appreciated ...sorry for my bad English

Thanks in Advance

[VictorN]

Class Button
{
void SetupButton()
}

//And it was declare at the Button.cpp as
vector<Button*>ButtonList

//It was use inside the SetupButton member function so that all available buttons in the game will be added in the list

void Button::SetupButton()
{
... //setup the property of the button
...//setup the position 
...//etc.

//then add to the list
ButtonList.push_back( this );
}

And in the game...
#include "Button.h"
Game::Game()
{
Button btnStartButton, btnExitButton;
btnStartButton.SetupButton();
btnExitButton.SetupExitButton();
}

Game::~Game()
{
ButtonList.clear();
... Delete some objects used
}

 Game* pGame
void Main()
{
   pGame = new Game()
   ...
   delete pGame;
    return;
}

1. What is SetupExitButton()?
2. What are these "some objects" that you delete in Game::~Game()? Where and how are they created?
3. Why create Game object in the heap (within the main() function!)?

[Paul McKenzie]

I'm wondering if what is the best approach to fix the problem.

Avoid using "new" as much as possible.

As Victor pointed out, your main() program is just a symptom of a much larger problem -- overusage of dynamically allocated objects. There is no need whatsoever to create an object using "new" in main(), and I can just bet that you're making the same type of coding in other parts of your library.

1) Use objects, not pointers.

2) If you need to use dynamically allocated memory, then use smart pointers (shared_ptr is one that exists with Visual Studio 2008 SP1 and above). Then memory cleanup is done automatically once the reference count for the smart pointer becomes 0, relieving you of doing this job.

#include <memory>
//...
typedef std::tr1::shared_ptr<Button> ButtonPointer;
//..
// if using VS 2010
// typedef std::shared_ptr<Button> ButtonPointer;
//...
vector<ButtonPointer> ButtonList;
//...
ButtonList.push_back(ButtonPointer(new Button));
//...

I cant use delete on every elements because they are not dynamically allocated.

If you give the user the rights to add pointers to your vector, and the pointers can come from anywhere, then that is a hole in your design -- you must have the responsibility of deletion thrown back to the creator of the object.

One way around this is to create a "button factory", where the user must ask for a button from the factory, the factory creates one, and gives it to the user. The user just can't say "new Button" or whatever to create a button -- they must use the factory to create one. This allows you to have total control how button objects are created and ultimately destroyed.

Regards,

Paul McKenzie

[himitsujanai]

1. What is SetupExitButton()?
2. What are these "some objects" that you delete in Game::~Game()? Where and how are they created?
3. Why create Game object in the heap (within the main() function!)?

Thanks for the reply

- Sorry for the confusion ... its a typo its supposed to be btnExitButton.SetupButton(). This is the function to be called to setup all the properties of the button and add them to the global vector ButtonList.

- "some objects" are member objects of game class such as settings (which contains configuration of the game ) and logger( that logs what happening the game, errors etc ). they are dynamically allocated and they are deleted in the destructor of the Game class these objects are not the cause of the problem it is the global variable ButtonList and additonal information all the elements of this vector is not dynamically allocated.

- actually its a int APIENTRY WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nCmdShow). "Why create Game object in the heap" well thats the game structure works even before I join the team and we've developed so many game using this design.

[Lindley]

- actually its a int APIENTRY WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nCmdShow). "Why create Game object in the heap" well thats the game structure works even before I join the team and we've developed so many game using this design.

As has been mentioned, there seems little point to dynamically allocating a single object in your entry point function. It's lifetime will be the entire length of the program anyway, so why bother with the trouble of dynamic allocation?

[Paul McKenzie]

they are dynamically allocated and they are deleted in the destructor of the Game class these objects are not the cause of the problem it is the global variable ButtonList and additonal information all the elements of this vector is not dynamically allocated.

Again, use a factory, where those objects that go into the vector aren't just "random" pointers from who-knows-where.

The design goal is to say "Only pointers that have been created this way can be stored in the vector". The way you do that is to make construction of the objects private, so that any code that attempts to create an object that violates your rules cannot be created. The compiler will flag any instance being created with "new", or even without "new".

So what goes in the place of the constructor is a static public member function (or a factory function) that creates the object and returns the pointer to the user. The user must call this function to create the object in question. That static member can be anything you want it to be -- in your case, you want to create the object using "new", then go ahead.

Regards,

Paul McKenzie

[Lindley]

Again, use a factory, where those objects that go into the vector aren't just "random" pointers from who-knows-where.

The design goal is to say "Only pointers that have been created this way can be stored in the vector". The way you do that is to make construction of the objects private, so that any code that attempts to create an object that violates your rules cannot be created. The compiler will flag any instance being created with "new", or even without "new".

So what goes in the place of the constructor is a static public member function (or a factory function) that creates the object and returns the pointer to the user. The user must call this function to create the object in question. That static member can be anything you want it to be -- in your case, you want to create the object using "new", then go ahead.

Regards,

Paul McKenzie

It's a perfectly valid design to document that a particular pointer is not responsible for cleaning up its content. That sounds like the case here.

If it's just the vector's own internal memory which is being reported as "leaking" (because it's global and the leak checker runs before globals are deconstructed), you could try using the swap trick to deallocate its memory prior to leak checking:

vector<Button*>().swap(ButtonList);

[himitsujanai]

Again, use a factory, where those objects that go into the vector aren't just "random" pointers from who-knows-where.

The design goal is to say "Only pointers that have been created this way can be stored in the vector". The way you do that is to make construction of the objects private, so that any code that attempts to create an object that violates your rules cannot be created. The compiler will flag any instance being created with "new", or even without "new".

So what goes in the place of the constructor is a static public member function (or a factory function) that creates the object and returns the pointer to the user. The user must call this function to create the object in question. That static member can be anything you want it to be -- in your case, you want to create the object using "new", then go ahead.

Regards,

Paul McKenzie

Thanks for your reply paul im sorry i missed to reply both of ur email...
- Yha i agree definitely if you can use object why pointers.
- as i said the elements that added in vector<Button*> ButtonList is not dynamically allocated the push_back function was called inside Button::SetupButton

void Button::SetupButton()
{
... //setup the property of the button
...//setup the position 
...//etc.

//then add to the list
ButtonList.push_back( this );

}

- And yes we used smart pointer in dynamically allocated pointers that we used
- I dont think we have a problem in the design. The design works pretty well except it reports 1 memory leak cause by this global vector

[himitsujanai]

It's a perfectly valid design to document that a particular pointer is not responsible for cleaning up its content. That sounds like the case here.

If it's just the vector's own internal memory which is being reported as "leaking" (because it's global and the leak checker runs before globals are deconstructed), you could try using the swap trick to deallocate its memory prior to leak checking:

vector<Button*>().swap(ButtonList);

"because it's global and the leak checker runs before globals are deconstructed"
Thanks Lindley i think you are right. As I said I already found a solution of this problem and it actually removed the reported leak but i want a shorter one which will not make me do a huge changes in my code.

Thanks for this line of code it is the most simplest solution that i can use.