7 replies to this topic

[PAndersson]

I'm currently working on a project, using C++, and have gotten stuck. The program has a memory leak you see, for which I have been unable to find the cause.

I have identified the object that causes it, and even narrowed it down to being being caused by it's constructor/destructor. But yet, I cannot for the life of me figure out why it leaks memory, everything it allocates dynamically is deleted. Yet it leaks memory. The leak is only 5 byte according to the tool I'm using, so I could very well ignore it and the project would not really suffer from it; but I so would like to understand why it is happening.

The Constructor in question

//Automatically sizes object to image size
guiButton::guiButton(char* name, int posX, int posY, int priority, gameImage* image, scriptFunction* function) 
{
	this->image = image;
	this->createObject(name, posX, posY, image->getSizeX(), image->getSizeY(), priority,0,0);
	this->resizeImage = false;
	this->guiButtonEvent = new gameEvent(gameEvent::eventGuiButtonClicked, this->id, function);
}

The Destructor

//Destructor
guiButton::~guiButton()
{
	std::cout << "GuiButton destructor!\n";
	delete this->guiButtonEvent;
}

And the objects header

//Simple button, clickable
class guiButton : public guiImage
{
protected:
	//Event that is flagged once button is clicked
	gameEvent* guiButtonEvent;
	guiButton() {}; //Does nothing
public:
	//Constructors
	//Automatically sizes object to image size
	guiButton(char* name, int posX, int posY, int priority, gameImage* image, scriptFunction* function);
	guiButton(char* name, int posX, int posY, int sizeX, int sizeY, int priority, gameImage* image, scriptFunction* function, bool resizeImage);
	guiButton(const guiButton& orig);
	//Creates a clone of this object, as if through a copy constructor. Deleting it needs to be done manually
	virtual guiObject* clone();
	//Attempt to interact with the object, flags an event in the passed manager if it was interacted with.
	void attemptInteraction(inputState* input, eventManager* manager) throw(std::exception*);
	//Destructor
	virtual ~guiButton();
};

And yes, the object is part of an inheritance hierarchy. But I have verified that it is at this level the leak occurs, as I can create guiImages without causing any kind of memory leak. guiImage also uses the exact
same call to the createObject function, so I doubt that is the cause, likewise gameEvent instances can be freely created without causing any kind of memory leaks.

Any help at all in figuring out what is going on would be greatly appreciated.

[Red Ant]

Have you considered using a smart pointer (e.g. boost::shared_ptr) implementation instead of manual memory management? That would reduce the chance of memory leaks by an order of magnitude. I would also encourage you to either ensure ALL your classes support proper copy semantics (i.e. sport correctly implemented copy assignment operators / copy constructors) or make them uncopyable - either by deriving from boost::noncopyable or by declaring the copy constructor / copy assignment operator private and not implementing them.

Also bear in mind that the tool you're using may just get confused about something and simply report a false positive.


EDIT: Let me expand on my point about copy semantics a bit.You have not implemented a copy assigment operator for your guiButton class, so it gets the compiler-generated default copy assigment operator, which does nothing more than perform a bit-wise copy of the source instance. Now consider the following:

guiButton a( "bla", 1, 2, 3, ptrToSomeImage, ptrToSomeScriptFunction );
guiButton b( "meepmeep", 5, 676, 2, ptrToAnotherImage, ptrToAnotherScriptFunction );

a = b; // <------- Memory leak here!!!!!!!

In the last line, a's guiButtonEvent pointer, which already pointed to a gameEvent object, simply gets overwritten with b's guiButtonEvent pointer, and the original guiButtonEvent pointer never gets freed.

[Hodgman]

As mentioned above, you're breaking the rule of 3. Accidental assignment could cause both a leak and a double-deletion (which you'd probably notice quickly).

What do your clone function and copy-constructor look like?

Is there any code that changes the value of guiButtonEvent during the lifetime of the object?

[PAndersson]

As mentioned above, you're breaking the rule of 3. Accidental assignment could cause both a leak and a double-deletion (which you'd probably notice quickly).

What do your clone function and copy-constructor look like?

Is there any code that changes the value of guiButtonEvent during the lifetime of the object?

While I certainly should overload the copy-assignment operator, I'm certain that is not them problem. For test purposes, I have made sure that no instances of any guiObjects are created save these three line:

delete new guiButton("W",1,1,1,imageResourceTracker->getItemByName("Art/Interface/moneyIco.tga"), NULL);
delete new guiButton("W",1,1,1,imageResourceTracker->getItemByName("Art/Interface/moneyIco.tga"), NULL);
delete new guiImage("W",1,1,1,imageResourceTracker->getItemByName("Art/Interface/moneyIco.tga"));

If I remove the first two lines, both detected memory leaks disappear. The third lines does not affect this, I can add and remove it to my hearts content.

As requested, my clone and copy constructor methods looks like this:

guiButton::guiButton(const guiButton& orig)
{
	//Copy by value
	this->guiButtonEvent = new gameEvent(*orig.guiButtonEvent);
	//
	this->copyCommonData(orig);
	this->resizeImage = orig.resizeImage;
	this->image = orig.image;
}

//Creates a clone of this object, as if through a copy constructor. Deleting it needs to be done manually
guiObject* guiButton::clone()
{
	guiButton* clone = new guiButton(*this);
	return clone;
}

Also, thanks for the help so far!

[Hodgman]

Some more ideas:
Is guiImage's destructor virtual?
Could it be that the leak is actually inside gameEvent's constructor/destructor?

[iMalc]

That is not how you should write a constructor in C++. The constructor should look more like this:

//Automatically sizes object to image size
guiButton::guiButton(char* name, int posX, int posY, int priority, gameImage* image, scriptFunction* function)
    : image(image), resizeImage(false)
{
    createObject(name, posX, posY, image->getSizeX(), image->getSizeY(), priority, 0, 0);
    guiButtonEvent = new gameEvent(gameEvent::eventGuiButtonClicked, id, function);
}

This is called a constructor initialisation list and it is both more efficient and shorter. In fact certain members can only be initialised this way, so you should learn about it sooner rather than later.

I take it that the std::cout in your destructor is temporary. It should normally look more like this:

guiButton::~guiButton()
{
    delete guiButtonEvent;
}

However, ideally the guiButtonEvent member would be some kind of smart pointer, rendering any kind of destructor within this class totally unnecessary. This would also help to prevent any leaks within the destructor where multiple calls to new are made and one of them fails.

As for your guiButton class, there are many sins present.
1. If there were ten commandments for writing your own class, following the rule of three would be number 1.
2. Don't write an empty constructor. Either leave the constructor out, or actually initialise the variables. At the moment code such as this would cause a crash due to deleting an uninitialised pointer:

{  guiButton a; }

3. The char* name parameters should at least be const char *name.
4. Your clone function should actually be marked as const, like this:

virtual guiObject* clone() const;

This indicates that it does not modify the object being cloned, and also allows you to clone a const object.
5. As indicated by your throw specifier, you are throing a pointer to an exception. Don't do this. Proper practice is to throw by value and catch by (const) reference.
6. Although exception specifiers were introduced into the language, common consensus is that their current existence is pretty much a mistake and they should actually not be used, except perhaps in the case of specifying that nothing is thrown, but even then the general rule is to not use them. Refer to the Guru of the Week (GotW) website to find out more.

[Hodgman]

And yes, the object is part of an inheritance hierarchy. But I have verified that it is at this level the leak occurs, as I can create guiImages without causing any kind of memory leak. guiImage also uses the exact same call to the createObject function, so I doubt that is the cause, likewise gameEvent instances can be freely created without causing any kind of memory leaks.

Does guiImage's default constructor call createObject (meaning it's called twice when constructing a guiButton)? What does that function do?

[PAndersson]

I'm well aware that it's not exatcly well written, I'm still in the process of learning. Rewriting large parts of it might be in order...

Does guiImage's default constructor call createObject (meaning it's called twice when constructing a guiButton)? What does that function do?

It does, and there was the cause. Thank you. It was a while since I last wrote it, I was sure it did do nothing but apparently it did call create object which copies a source string into a new, dynamically allocated, string. If you call it twice, the old one never gets deleted...

Thanks a lot for the help!