Back to General and Gameplay Programming

memory leak, don't know where to call delete.

General and Gameplay Programming Programming

Started by JinJo June 27, 2008 11:57 AM

17 comments, last by Gage64 15 years, 9 months ago

JinJo

100

Author

June 27, 2008 11:57 AM

Hi there, I just started using Paul Nettle's Memory Manager and detected a leak in my code. Here is the culprit function:

[Source]
void InputSystem::GenerateEvent(std::string newEvent)
{
	InputEvent *e;
	if(newEvent == "KeyDown:D_Key")
	{
		e = new InputEvent("D");
	}
	if(newEvent == "KeyDown:A_Key")
	{
		e = new InputEvent("A");
	}
	if(e)
	{	
		events.push_back(e);
	}
		
}
[/Source]

events is a vector which the input event gets stored in. (note all this may be because I don't know how to use vectors properly?) Here is where I try and delete the event (events.clear() at bottom)


void InputSystem::HandleEvents()
{
	std::vector<EventHandler*>::iterator h;
	std::vector<InputEvent*>::iterator e;

	
	for(h=handlers.begin(); h!=handlers.end();)
	{
		for(e=events.begin(); e!=events.end();)
		{
			InputEvent *ev = (*e);
			e++;
			(*h)->handleEvent(ev);
			delete ev;
			
		}
		h++;
	}
	events.clear();
}

However it doesn't seem to delete the Input Event memory that was created in the other function. Anyone tell me whats going on here? thanks.

rip-off

11,000

June 27, 2008 12:12 PM

I don't see why your input events are allocated dynamically at all - unless there is other code interacting with the vector you haven't posted?

blog | twitter

visitor

643

June 27, 2008 12:22 PM

One problem is that you are pushing uninitialized pointers into events in InputSystem::GenerateEvent. You need to initialize the pointer to NULL (0) if you want to check whether it was assigned something to.

And a safer solution against memory leaks (especially in face of exceptions) might be to use a more appropriate container for storing dynamically allocated objects, such as boost::ptr_vector.

Edit: unless dynamic allocations are necessary in the first place.

ukdeveloper

264

June 27, 2008 12:36 PM

I'm with rip-off here, is there any reason for you to be using pointers to do this in any case? Bear in mind that the local pointer *e in GenerateEvent() can only be deleted internally but, as you're not doing so, you're effectively ending up with a spare copy which you don't need (or at least as far as I can see that's your problem).

I see no need at all to use pointers unless you're not giving us the full picture of what you're trying to do.

Ravyne

14,306

June 27, 2008 12:41 PM

Quote:Original post by ukdeveloper
I'm with rip-off here, is there any reason for you to be using pointers to do this in any case?

...

I see no need at all to use pointers unless you're not giving us the full picture of what you're trying to do.

Agreed.

Quote:Bear in mind that the local pointer *e in GenerateEvent() can only be deleted internally but, as you're not doing so, you're effectively ending up with a spare copy which you don't need (or at least as far as I can see that's your problem).

Come again? I think you might want to rethink that statement [wink]

You have some much greater issues here in HandleEvents. You are stepping through all events for each handler and deleting them. Guess what happens when you have more than one handler? You give it a dangling pointer to a deleted InputEvent, that's what. When you delete a pointer, you should always set it to null, in this case that means both 'ev' and '*e' need to be nullified.

Also, you can move the 'h++' and 'e++' out into the 'for(...)' where it belongs, and it should be '++h' and '++e' since you have no need to preserve the pre-step value.

I don't see anything wrong with the deletion itself, except that it's location flies in the face of your apparent intention [grin]. If you want all handlers to receive all events, you need to delete the events *after* you've run them through all the handlers.

[Edited by - Ravyne on June 27, 2008 12:41:22 PM]

throw table_exception("(? ???)? ? ???");

ukdeveloper

264

June 27, 2008 12:54 PM

Quote:Original post by Ravyne
Come again? I think you might want to rethink that statement [wink]

I think I've confused myself because my C++ is a little bit rusty... my understanding is that he is allocating the memory inside the function, the function terminates and the pointer goes out of scope but the memory is still allocated, but with no way to access it - memory leak.

Although I'm fully expecting to be 110% wrong on this one [grin]

Gage64

1,235

June 27, 2008 12:57 PM

Quote:Original post by ukdeveloper
Quote:Original post by Ravyne
Come again? I think you might want to rethink that statement [wink]

I think I've confused myself because my C++ is a little bit rusty... my understanding is that he is allocating the memory inside the function, the function terminates and the pointer goes out of scope but the memory is still allocated, but with no way to access it - memory leak.

Although I'm fully expecting to be 110% wrong on this one [grin]

He is pushing a copy of the pointer into the vector, so even if the local pointer goes out of scope, the one in the vector doesn't. Therefore the memory can be deleted through the pointer in the vector.

OOP Articles | C++: A Dialog (free book) | Thinking in C++ (free book) | Google Books - Free preview for many books

Ravyne

14,306

June 27, 2008 12:59 PM

Quote:Original post by ukdeveloper
Quote:Original post by Ravyne
Come again? I think you might want to rethink that statement [wink]

I think I've confused myself because my C++ is a little bit rusty... my understanding is that he is allocating the memory inside the function, the function terminates and the pointer goes out of scope but the memory is still allocated, but with no way to access it - memory leak.

Although I'm fully expecting to be 110% wrong on this one [grin]

As long as you have a copy of the pointer, which is what he stores in the vector, then you can delete it at any time. The whole point of new/delete is so that you can create dynamic objects which can live across scope boundaries.

throw table_exception("(? ???)? ? ???");

mzeo77

168

June 27, 2008 01:16 PM

You need to initialize e to 0 as otherwise it may be random data.

	InputEvent *e = 0;

	for(h=handlers.begin(); h!=handlers.end();)	{		for(e=events.begin(); e!=events.end();)		{			InputEvent *ev = (*e);			e++;			(*h)->handleEvent(ev);			// Not good if there are more than one handler			// need to wait delete until all handlers are taken			// care of			delete ev;					}		h++;	}	events.clear();

This is how it should look like:

	...	// First handle all evens by all handles	for(h=handlers.begin(); h!=handlers.end(); ++h)	{		for(e=events.begin(); e!=events.end(); ++e)			(*h)->handleEvent(*e);	}	// Then delete them	for(e=events.begin(); e!=events.end(); ++e)		delete *e;	events.clear();

the_edd

2,109

June 27, 2008 01:53 PM

99% of the time, if you write code in which you have to invoke delete or delete[] manually, you're doing it rong. This is a generalisation of rip-off's and vistor's posts.

Structure your code so that you don't have to worry.

http://www.mr-edd.co.uk
http://bitbucket.org/edd

memory leak, don't know where to call delete.

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

memory leak, don't know where to call delete.

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines