Jump to content
  • Advertisement
Sign in to follow this  
JinJo

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

This topic is 3646 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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.

Share this post


Link to post
Share on other sites
Advertisement
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?

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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]

Share this post


Link to post
Share on other sites
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]

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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();


Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

Participate in the game development conversation and more when you create an account on GameDev.net!

Sign me up!