bool GFX::Update(Collision_Handler* Test_Against)
{
Collision_Relay* Collision = new Collision_Relay(0, 0);// creats a sort of information package to be sent to the current sprite about what its hitting(if anything)
for(int i = 0; i < Sprites.size(); i++)//loops through all current sprites
{
Sprites->Update();//just handles animation
Off_Set_Blit(Sprites);//draws the sprites based on an offset value
Collision = Test_Against->Check_All(Sprites, this);// fill that packet mentioned above with relative information about that sprite
if(Collision != NULL)
Sprites->Status = Collision;//essentially deliver that imaginary collision info package
if( Sprites->Image == NULL)//if it doesnt exist
{
return false;//we have a problem
};
if(Sprites->A_Speed == -1)//just a delete code i build in to this function its pretty much a delete flag
{
Sprites.erase(Sprites.begin() + i);//remove that sprite from the vector
};
};
delete Collision;//some clean up code
Collision = 0;
return true;//everything went smoothly if we have reached this point
};
Spot the memory leak
Author
ok i know that there is a memory leak in this function and even after pouring over it far at least an hour maybe 2 i still kind find anything, in short this code need to be looked at with some fresh eyes so here it is
if there is a certine function called within this one that could be the culprate i will post it but i dont want to clutter up this post with all my class/function declerations
thanks alot =)
bool GFX::Update(Collision_Handler* Test_Against) { Collision_Relay* Collision = new Collision_Relay(0, 0);// creats a sort of information package to be sent to the current sprite about what its hitting(if anything) for(int i = 0; i < Sprites.size(); i++)//loops through all current sprites { Sprites->Update();//just handles animation Off_Set_Blit(Sprites);//draws the sprites based on an offset value Collision = Test_Against->Check_All(Sprites, this);// fill that packet mentioned above with relative information about that sprite if(Collision != NULL) Sprites->Status = Collision;//essentially deliver that imaginary collision info package if( Sprites->Image == NULL)//if it doesnt exist {delete Collision; //add me here! return false;//we have a problem WE SURE DO, THIS IS THE SOURCE OF YOUR LEAK }; if(Sprites->A_Speed == -1)//just a delete code i build in to this function its pretty much a delete flag { Sprites.erase(Sprites.begin() + i);//remove that sprite from the vector }; }; delete Collision;//some clean up code Collision = 0; return true;//everything went smoothly if we have reached this point };
ooh wait, there are others....
Ugh... this is an ugly function. You need to study up on pointers and dynamic allocation a bit. Grab a good book, or just Google it up.
bool GFX::Update(Collision_Handler* Test_Against) { Collision_Relay* Collision = new Collision_Relay(0, 0);// creats a sort of information package to be sent to the current sprite about what its hitting(if anything) for(int i = 0; i < Sprites.size(); i++)//loops through all current sprites { Sprites->Update();//just handles animation Off_Set_Blit(Sprites);//draws the sprites based on an offset value delete collision; //HERE ALSO!! Collision = Test_Against->Check_All(Sprites, this);// fill that packet mentioned above with relative information about that sprite if(Collision != NULL) Sprites->Status = Collision;//essentially deliver that imaginary collision info package if( Sprites->Image == NULL)//if it doesnt exist { delete collision; return false;//we have a problem }; if(Sprites->A_Speed == -1)//just a delete code i build in to this function its pretty much a delete flag { Sprites.erase(Sprites.begin() + i);//remove that sprite from the vector }; }; delete Collision;//some clean up code Collision = 0; return true;//everything went smoothly if we have reached this point };Ugh... this is an ugly function. You need to study up on pointers and dynamic allocation a bit. Grab a good book, or just Google it up.
"certain"
"culprit"
The only thing which obviously allocates memory here is the call to "new".
First, you test "Collision" against NULL -- but it will never be NULL; "new" will throw if it runs out of memory.
Then, you assign this collision pointer to a sprite member variable. However, you don't actually call something on the sprite to process this data. You also don't do anything with what the pointer might previously have been. This is likely a logic bug -- the Sprites->Status variable will be a stale pointer after the final delete.
Also, you return false if the sprite has no image. However, at that point, you don't delete the Collision_Relay object -- that's a leak, for sure.
You might be better off just creating the Collision_Relay object on the stack:
There's a third problem, where you don't adjust the "i" value after you delete an item from the vector. This means that the item behind whatever is being deleted, will not be processed during that call to Update(). In general, deleting from within a vector, while you're iterating over that vector, is tricky, and you can often do better by just marking an item dirty, and then deleting during a second pass (in that pass, walk the vector from the end to the beginning to avoid the index invalidation problem).
You probably meant to put the call to Sprites->Update() and the call to Off_Set_Blit() last in the inner loop, so that the collision data you've delivered actually is used (presumably by Update()) before being deleted.
"culprit"
The only thing which obviously allocates memory here is the call to "new".
First, you test "Collision" against NULL -- but it will never be NULL; "new" will throw if it runs out of memory.
Then, you assign this collision pointer to a sprite member variable. However, you don't actually call something on the sprite to process this data. You also don't do anything with what the pointer might previously have been. This is likely a logic bug -- the Sprites->Status variable will be a stale pointer after the final delete.
Also, you return false if the sprite has no image. However, at that point, you don't delete the Collision_Relay object -- that's a leak, for sure.
You might be better off just creating the Collision_Relay object on the stack:
Collision_Relay CollisionInstance( 0, 0 ), *Collision = &CollisionInstanceThere's a third problem, where you don't adjust the "i" value after you delete an item from the vector. This means that the item behind whatever is being deleted, will not be processed during that call to Update(). In general, deleting from within a vector, while you're iterating over that vector, is tricky, and you can often do better by just marking an item dirty, and then deleting during a second pass (in that pass, walk the vector from the end to the beginning to avoid the index invalidation problem).
You probably meant to put the call to Sprites->Update() and the call to Off_Set_Blit() last in the inner loop, so that the collision data you've delivered actually is used (presumably by Update()) before being deleted.
Author
ok thanks for the help, I did know that there was that test against null and was going to take it out i just left it in (Stupid *hits self*)
I think i will go for a second pass idea to clean stuff up. Another thing that i guess i didnt make entirely clear was that the Test_Against was a pointer to my a Collision detection class instance that checks the sprite agains all objects in the scene to check for collision but the fact that i had "Collision" in there at all was redundant on my part (just giving me more to wory about) because i could have just assigned status directly to
Test_Against->Check_All(Sprites, this); and status DOES get cleared every frame at the end of Update() to be sure that all nessisary ajustments are made based on Status before it gets deleted thank you both very much for your help.
I think i will go for a second pass idea to clean stuff up. Another thing that i guess i didnt make entirely clear was that the Test_Against was a pointer to my a Collision detection class instance that checks the sprite agains all objects in the scene to check for collision but the fact that i had "Collision" in there at all was redundant on my part (just giving me more to wory about) because i could have just assigned status directly to
Test_Against->Check_All(Sprites, this); and status DOES get cleared every frame at the end of Update() to be sure that all nessisary ajustments are made based on Status before it gets deleted thank you both very much for your help.
Quote:First, you test "Collision" against NULL -- but it will never be NULL; "new" will throw if it runs out of memory.
"blanket"
"assumption"
- You are assuming that the person is not using their own memory manager which might return NULL when out of memory
- You are assuming that the person is handling exceptions (with no NULL check, and no exception handler, what happens when the release mode is compiled)
When getting people to think more deeply about the use of dynamic memory, it is generally best to not make assumptions, or to at least clarify those assumptions.
Quote:Original post by Tesseract_HotplateQuote:First, you test "Collision" against NULL -- but it will never be NULL; "new" will throw if it runs out of memory.
"blanket"
"assumption"
- You are assuming that the person is not using their own memory manager which might return NULL when out of memory
- You are assuming that the person is handling exceptions (with no NULL check, and no exception handler, what happens when the release mode is compiled)
When getting people to think more deeply about the use of dynamic memory, it is generally best to not make assumptions, or to at least clarify those assumptions.
The first assumption seems reasonable. The keyword 'new' is clearly used, and unless the person overloaded operator new for the Collision_Relay class, or global operator new, then he obviously can't be using his own memory manger, because placement new isn't being called. I think, given the nature of the question, that it is a very reasonable assumption, and one that needed no clarification.
For your second statement, I have no idea what you are talking about. What does handling exceptions have to do with anything? Just because you don't handle the possibility of an exception doesn't mean new won't throw if it can't allocate memory.
The only thing I see him assuming is that new will throw, which, it should, but most older compilers don't implement it that way.
Quote:Original post by mfawcett
The first assumption seems reasonable. The keyword 'new' is clearly used, and unless the person overloaded operator new for the Collision_Relay class, or global operator new, then he obviously can't be using his own memory manger, because placement new isn't being called. I think, given the nature of the question, that it is a very reasonable assumption, and one that needed no clarification.
You honestly think that if you use the keyword 'new', you can't be using your own implementation? I assure you that you are wrong. 'new' is an operator which can be overloaded.
Quote:For your second statement, I have no idea what you are talking about. What does handling exceptions have to do with anything? Just because you don't handle the possibility of an exception doesn't mean new won't throw if it can't allocate memory.
Ask yourself what happens if your program throws an unhandled exception. Do you really want the whole thing crashing down? What if you can handle a NULL return from a memory allocator in a different way?
The purpose wasn't to get bogged down in semantics. It was to show how tricky it can get when one makes blanket assumptions, particularly when they have nothing to do with the question asked. Checking the return from the allocation doesn't affect the situation - that of a memory leak - at all.
Quote:Original post by Tesseract_Hotplate
You honestly think that if you use the keyword 'new', you can't be using your own implementation? I assure you that you are wrong. 'new' is an operator which can be overloaded.
You quoted me so I assume you read what I typed, but apparently you missed some vital parts of my statements, especially the ones where I mentioned overloading operator new for the Collision_Relay class, or even the global operator new. Go back and reread and you'll see why your response has left me baffled.
Quote:Original post by Tesseract_Hotplate
Ask yourself what happens if your program throws an unhandled exception. Do you really want the whole thing crashing down? What if you can handle a NULL return from a memory allocator in a different way?
The purpose wasn't to get bogged down in semantics. It was to show how tricky it can get when one makes blanket assumptions, particularly when they have nothing to do with the question asked. Checking the return from the allocation doesn't affect the situation - that of a memory leak - at all.
I know exactly what happens in the event of an unhandled exception. Your previous post was talking about assuming the user handled exceptions, which, I will re-iterate, has absolutely nothing to do with the problem(s) at hand.
Also, if a memory manager is being used that overloads global operator new, only the nothrow version should return null. Any memory manager which fails to throw an exception with the "normal" operator new is horribly flawed IMHO.
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement