Sign in to follow this  
raptorstrike

Spot the memory leak

Recommended Posts

raptorstrike    181
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
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[i]->Update();//just handles animation 
         Off_Set_Blit(Sprites[i]);//draws the sprites based on an offset value
         Collision = Test_Against->Check_All(Sprites[i], this);// fill that packet mentioned above with relative information about that sprite
         if(Collision != NULL)
            Sprites[i]->Status = Collision;//essentially deliver that imaginary collision info package
         if( Sprites[i]->Image == NULL)//if it doesnt exist
         {
             return false;//we have a problem
         };
         if(Sprites[i]->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
 };




thanks alot =)

Share this post


Link to post
Share on other sites
SnprBoB86    277

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 &lt; Sprites.size(); i++)//loops through all current sprites
{
Sprites[i]-&gt;Update();//just handles animation
Off_Set_Blit(Sprites[i]);//draws the sprites based on an offset value
Collision = Test_Against-&gt;Check_All(Sprites[i], this);// fill that packet mentioned above with relative information about that sprite
if(Collision != NULL)
Sprites[i]-&gt;Status = Collision;//essentially deliver that imaginary collision info package
if( Sprites[i]-&gt;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[i]-&gt;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
};


Share this post


Link to post
Share on other sites
SnprBoB86    277
ooh wait, there are others....



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 &lt; Sprites.size(); i++)//loops through all current sprites
{
Sprites[i]->Update();//just handles animation
Off_Set_Blit(Sprites[i]);//draws the sprites based on an offset value
delete collision; //HERE ALSO!!
Collision = Test_Against->Check_All(Sprites[i], this);// fill that packet mentioned above with relative information about that sprite
if(Collision != NULL)
Sprites[i]->Status = Collision;//essentially deliver that imaginary collision info package
if( Sprites[i]->Image == NULL)//if it doesnt exist
{
delete collision;
return false;//we have a problem
};
if(Sprites[i]->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.

Share this post


Link to post
Share on other sites
hplus0603    11356
"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[i]->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 = &CollisionInstance;



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[i]->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.

Share this post


Link to post
Share on other sites
raptorstrike    181
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[i], 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.

Share this post


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

Share this post


Link to post
Share on other sites
mfawcett    373
Quote:
Original post by Tesseract_Hotplate
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.


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.

Share this post


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

Share this post


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

Share this post


Link to post
Share on other sites
Quote:

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.


What I find baffling is how you seem to think one cannot use an overloaded operator new without using placement new or specifically implementing it for a class. Again, I assure you that you are wrong. The code reproduced above could easily be using a custom new implementation with all the gory details behind the scenes.

Quote:
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.


And I will re-iterate that the entire discussion has nothing to do with the problem(s) at hand. The OP asked for help spotting a memory leak and received a response of "don't check for NULL returning from new because I say you shouldn't". My responses have been to illustrate that the assumptions behind that statement are flawed, and that when a person asks for help with a particular area of their code time is not best spent critiquing minor details that one may not even fully understand. Instead, why not look at the problem the person is asking about.

Share this post


Link to post
Share on other sites
mfawcett    373
Quote:
Original post by Tesseract_Hotplate
What I find baffling is how you seem to think one cannot use an overloaded operator new without using placement new or specifically implementing it for a class. Again, I assure you that you are wrong. The code reproduced above could easily be using a custom new implementation with all the gory details behind the scenes.


Okay, now I see the problem. Your reading comprehension failed, so we had a misunderstanding. I'll re-quote what I originally typed for you, and then expound upon it to perhaps clear up your [mis]-understanding. Is English not your first language perhaps?

Here are my original sentences, with further explanations.

Quote:
The first assumption seems reasonable.

This sentence means that I believe that it is safe to assume that the OP is not using a custom memory manager.

Quote:
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 this is the sentence you are having trouble with. I list out the facts, assumptions, and conclusion in one sentence. Given the confusion this has caused upon you it seems I should've broken this into parts.

1. The keyword 'new' is being used (it is evident in his code).
2. The OP does not mention overloading operator new for the Collision_Relay class.
3. The OP does not mention overloading global new (or redefining the keyword ala MFC).
4. placement new is clearly not being used.

Quote:
I think, given the nature of the question, that it is a very reasonable assumption, and one that needed no clarification.

I then re-iterate the conclusion: The OP is using the plain old 'new' operator.


Quote:
Original post by Tesseract_Hotplate
And I will re-iterate that the entire discussion has nothing to do with the problem(s) at hand. The OP asked for help spotting a memory leak and received a response of "don't check for NULL returning from new because I say you shouldn't". My responses have been to illustrate that the assumptions behind that statement are flawed, and that when a person asks for help with a particular area of their code time is not best spent critiquing minor details that one may not even fully understand. Instead, why not look at the problem the person is asking about.


Certainly, that is why I mentioned that although 'new' should throw, a lot of older compilers don't behave that way, and NULL could certainly be returned. I think it might help if you don't skim my responses, and instead, please take the time to actually read them.

Share this post


Link to post
Share on other sites
Quote:
Okay, now I see the problem. Your reading comprehension failed


Come now. Just because I've demonstrated a greater understanding of the use of overloaded operators in C++ than you, you resort to name calling?

Perhaps that shouldn't be surprising, since you only joined this conversation to argue with me, not to help the OP.

Quote:
This sentence means that I believe that it is safe to assume that the OP is not using a custom memory manager.


Then that is the end of the discussion. Because you admit that it is an assumption. Furthermore, this assumption was used to make a point completely irrelevant to the topic at hand.

Quote:

1. The keyword 'new' is being used (it is evident in his code).

- Which means nothing.
Quote:

2. The OP does not mention overloading operator new for the Collision_Relay class.

- Which means nothing. You should really get a C++ book.
Quote:

3. The OP does not mention overloading global new (or redefining the keyword ala MFC).

- See previous
Quote:

4. placement new is clearly not being used.

- And again with the placement new. Seriously, read a C++ book.

Quote:
I then re-iterate the conclusion: The OP is using the plain old 'new' operator.


I then reiterate my conclusion: you do not know what the OP is using, and advising the user regarding how to handle return values from 'new' is both pointless and presumptive. Quick, tell me how deleting the line that checks the return value from 'new' will resolve the memory leak? I'll wait.

Obviously you are just one of the many amateur coders who loves to hang out on boards, wait for someone to post code with a problem, and then pick it apart because it doesn't match your style or approach. Which is why people don't ask questions. Your type are actually detrimental to the whole process of sharing information and resolving problems.

Share this post


Link to post
Share on other sites
mfawcett    373
Sorry for derailing the thread. I assure you I am not amateur, and if you go through my past postings, you'll find I am extremely knowledgable about memory management and operator overloading, and try to be very helpful on these boards.

Could you kindly post an example of how the OP's code could possibly be using a custom memory manager? Remember, he hasn't overload the Collision_Relay class's operator new, he hasn't overloaded global operator new, he hasn't redefined the new keyword, and he isn't using placement new. I eagerly await your response.

Share this post


Link to post
Share on other sites
Quote:
Could you kindly post an example of how the OP's code could possibly be using a custom memory manager? Remember, he hasn't overload the Collision_Relay class's operator new, he hasn't overloaded global operator new, he hasn't redefined the new keyword, and he isn't using placement new. I eagerly await your response.


This is, obviously, a pointless argument. However I find it very interesting that you don't understand the problem with your statements.

All you have seen is a code snippet. You do not have the entire program before your eyes. From this code snippet you draw all the above conclusions, all of which could very easily be false. Whether or not the OP later confirmed them is irrelevant. At the time you - and the person who first critiqued checking the return value from new - made your assumptions, you did not have such confirmation of your assumptions.

One could just as easily state that the person's program doesn't work because he didn't define the class Collision_Relay, or Collision_Handler. Or because his entire engine appears to be one function. Both this pointless observation and the other pointless observation add nothing to the discussion, and require limiting thought to the code sample given.

In any case, since you asked:

In some file separate from code snippet:

inline void *operator new(unsigned int uiBytes, const char *pszFile, s32 iLine)
{
// Implementation ...
}
inline void *operator new[](unsigned int uiBytes, const char *pszFile, s32 iLine)
{
// Implementation ...
}
#define new new(__FILE__, __LINE__)


The code snippet above would then work, perfectly, using the new operator exactly as if it hadn't been overloaded.

The point being, you don't know what is going on behind the scenes of the code. Furthemore, the posted 'advice' was not even pertinent to the question being asked.

More than anything this reminds me of the people on comp.lang.c who needlessly pick apart code which is posted by the author to find some problem. Newbies will post their 'void main' functions with an obvious bug inside, and get 100 posts telling them 'void main is wrong, idiot, it doesn't exist' but not a single reply addressing their question.

Share this post


Link to post
Share on other sites
mfawcett    373
You spouted off insults about how you knew more about operator overloading, and how I was clearly wrong, yet all of your posts reiterate things I've said in my own posts, only you quote mine as wrong. It's very disconcerting to see someone so vehemently defend himself with things like:

Quote:
Original post by Tesseract_Hotplate
Quote:
Original post by mfawcettThe 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.

I mean, that is baffling. Where in my quote did you possibly get the idea that I did not know 'new' was an operator that could be overloaded. Was it perhaps me mentioning every way 'new' could possibly be replaced? I was literally dumbfounded by your reply, which is why I figured the only way that you could've reached that conclusion was if you misunderstood, if you skimmed, or if English was not your first language.

Later I asked you, please, pray tell, how is it possible to be using a custom memory manager when Collision_Relay class did not overload operator new, the global operator new was not overloaded, he did not redefine the new keyword, and did not use placement new. You then posted code to say that indeed it is possible, and that I am horribly mistaken, and that I should go read a C+ book. I was hoping for enlightenment, perhaps I'd learn something new, but your code 1. overloads the global operator new, and 2. redefines the new keyword.

All of my posts have said that yes, it is an assumption we made that the user was not overloading operator new (and I hope I don't need to list all the ways that that is possible again), however an assumption that I believe can be safely made in this context. The OP came to the forum asking about a memory leak. It seems like common sense that if you made your own memory manager, and are then having memory leak problems, you better mention to us that you are using a custom memory manager.

As the case may be, the user
1. Did not mention overloading Collision_Relay class's operator new
2. Did not mention overloading the global operator new
3. Did not mention redefining the new keyword
4. Did not use placement new

Perhaps before giving advice next time, we should ask the OP if he is using his own implementation of the STD library as well? I mean, let's not make assumptions, right?

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this