Sign in to follow this  
muffinsssss

memory leak

Recommended Posts

Hey, so I seem to have a small memory leak. Basically, I'm creating an object, then I want it to destroy itself after so many frames. Here is the code where the object is made, along with the code for destroying itself.

//where object is created:

GDamageBox* box = new GDamageBox;
box->OnLoad("bin/images/db.bmp", 64, 64, 1);
box->SetSource(this,100,0);  
GObject::ObjectList.push_back(box);   //static list of game objects


//where object is destroyed:

std::vector<GObject*>::iterator it = GObject::ObjectList.begin();

while(*it!=this)
   advance(it,1);

delete *it;  //also tried "delete this"
GObject::ObjectList.erase(it);

//destructor

GDamageBox::~GDamageBox() {
    SourceObj = NULL;   //we don't delete SourceObj, as its still being used!
}

Everything looks like it works as it should in game....but my physical memory usage goes up and up the more of these objects I create. Also, I have checked that it is in fact removing the object from the object vector, as it is no longer being rendered. Also, the destructor is being called, I checked. Any ideas? SideNote: Doesn't seem to matter whether or not the "delete *it" / "delete this" line is there at all, the program runs the same either way. [Edited by - jpetrie on August 17, 2009 9:47:46 PM]

Share this post


Link to post
Share on other sites
Quote:
Original post by BLiTZWiNG
I haven't done C++ in quite some time, but shouldn't it be "delete it"? (without the *)
No. 'it' is an iterator to a std::vector<GObject*>. If you did 'delete it;' and your program compiled I'm kind of surprised. Unless there's some sort of overload to 'it' that allows for that.

I am concerned about this:
GDamageBox::~GDamageBox() {
SourceObj = NULL; //we don't delete SourceObj, as its still being used!
}
When is it NOT used that you DO delete it? What IS it? Also, if it's a member variable of your object then you don't need to set it to NULL. It is destroyed RIGHT after that after all.

edit: While I'm here I am concerned about some other things you do. For instance, after creating a new GDamageBox (what is the G for? I don't think it could mean global because GDamageBox and GObject are both types. Seems entirely unnecessary and as it confuses at least one person--me--it probably doesn't do what you want it to)

If you also call "OnLoad" and "SetSource" after creating a GDamageBox then just make a constructor that looks like this when called[code]GDamageBox * box = new GDamageBox("bin/images/db.bmp", 64, 64, 1, this, 100, 0);[code]only maybe with a better order for the arguments.

I have to wonder why you think having a static list of game objects is helpful at all. Maybe you are trying to implement your idea of a singleton? You probably designed yourself into a corner where that seemed like the best solution to whatever problem you were having, but I would avoid that in the future if I were you and get rid of it if possible.

Share this post


Link to post
Share on other sites
Quote:
I haven't done C++ in quite some time, but shouldn't it be "delete it"? (without the *)
There may be other problems with the OP's code (I haven't looked carefully), but delete *it is correct.

Share this post


Link to post
Share on other sites
Quote:
Original post by BLiTZWiNG
I haven't done C++ in quite some time, but shouldn't it be "delete it"? (without the *)


No, "it" is an iterator not a pointer, to get the pointer (or a reference to the contained object) you use *it.

What is advance?

try this:


std::vector<GObject*>::iterator it = std::find(GObject::ObjectList.begin(),GObject::ObjectList.end(), this);

if(it!=GObject::ObjectList.end())
{
delete *it; //also tried "delete this"
GObject::ObjectList.erase(it);
}
else
{
std::cout << "Object not in list" << std::endl;
}
//destructor

GDamageBox::~GDamageBox() {
SourceObj = NULL; //we don't delete SourceObj, as its still being used!
}






Not sure if you're supposed to be destroying objects inside their own methods...

Edit: Yeah, you can call "delete this" but can't access any variables or methods after you do, definitely should not be calling delete this inside the destructor.

Share this post


Link to post
Share on other sites
Quote:
Original post by nobodynews
I am concerned about this:
GDamageBox::~GDamageBox() {
SourceObj = NULL; //we don't delete SourceObj, as its still being used!
}
When is it NOT used that you DO delete it? What IS it? Also, if it's a member variable of your object then you don't need to set it to NULL. It is destroyed RIGHT after that after all.


SourceObj is the object this object "follows". This object is created by SourceObj, but SourceObj has no control over it, so its not a member variable of SourceObj. SourceObj in its current state is the player, the one you control, and isn't destroyed until the game is closed.

Quote:
Original post by nobodynews
edit: While I'm here I am concerned about some other things you do. For instance, after creating a new GDamageBox (what is the G for? I don't think it could mean global because GDamageBox and GObject are both types. Seems entirely unnecessary and as it confuses at least one person--me--it probably doesn't do what you want it to)

If you also call "OnLoad" and "SetSource" after creating a GDamageBox then just make a constructor that looks like this when called[code]GDamageBox * box = new GDamageBox("bin/images/db.bmp", 64, 64, 1, this, 100, 0);[code]only maybe with a better order for the arguments.

The G just stands for game, and is useless yes. The other constructor related things I play to go back and improve, the project is still in the very early stages.

Quote:
Original post by nobodynews
I have to wonder why you think having a static list of game objects is helpful at all. Maybe you are trying to implement your idea of a singleton? You probably designed yourself into a corner where that seemed like the best solution to whatever problem you were having, but I would avoid that in the future if I were you and get rid of it if possible.

I am following the game tutorial at www.sdltutorials.com. I am still very new to C++ (2 months now) and am using this mostly as a learning exercise. As far as the design of the game...well I didn't design the whole static objects thing, that's just what he does.

Share this post


Link to post
Share on other sites
Quote:
Original post by Kwizatz
Quote:
Original post by BLiTZWiNG
I haven't done C++ in quite some time, but shouldn't it be "delete it"? (without the *)


No, "it" is an iterator not a pointer, to get the pointer (or a reference to the contained object) you use *it.

What is advance?

try this:

*** Source Snippet Removed ***

Not sure if you're supposed to be destroying objects inside their own methods...

Edit: Yeah, you can call "delete this" but can't access any variables or methods after you do, definitely should not be calling delete this inside the destructor.


I changed it so that I was destroying the object in the main loop of the program. Now I have it checking to see if an object is dead, if so, it removes it. Still same issue... =[[[


for(int i = 0;i < GObject::ObjectList.size();i plus plus) {
if(!GObject::ObjectList[i]) continue;

GObject::ObjectList[i]->OnLoop();

if(GObject::ObjectList[i]->Dead) {
delete GObject::ObjectList.at(i);
GObject::ObjectList.erase(GObject::ObjectList.begin() plus i);
}
}


+ signs weren't working, so I wrote out plus....not sure why that is?

Share this post


Link to post
Share on other sites
std::vector may not reallocate memory each time you erase an element and keep the memory used by erased nodes to allocate to newly added elements, the memory usage increment may come from that.

As for the object, if the destructor is called, then its being deleted.

So, diagnosing a memory leak based only in memory usage may not be accurate, try Visual Studio's Built in memory leak detector or Valgrind if you're using Linux.

Edit: Actually, the VC leak detection seems to be a pain to use, not sure if CodeAnalyst detects memory leaks, it should.

Share this post


Link to post
Share on other sites
Quote:
Original post by Kwizatz
std::vector may not reallocate memory each time you erase an element and keep the memory used by erased nodes to allocate to newly added elements, the memory usage increment may come from that.

As for the object, if the destructor is called, then its being deleted.

So, diagnosing a memory leak based only in memory usage may not be accurate, try Visual Studio's Built in memory leak detector or Valgrind if you're using Linux.

Edit: Actually, the VC leak detection seems to be a pain to use, not sure if CodeAnalyst detects memory leaks, it should.

Thanks a bunch, I'll look into that!

Share this post


Link to post
Share on other sites
Quote:
Original post by Kwizatz
Edit: Actually, the VC leak detection seems to be a pain to use



// All translation units must include:

#if defined( _MSC_VER )
#define _CRTDBG_MAP_ALLOC
#include <stdlib.h>
#include <crtdbg.h>
#define new DEBUG_NEW
#define DEBUG_NEW new( _NORMAL_BLOCK, __FILE__, __LINE__ )
#endif

// And add this at main()
_CrtSetDbgFlag ( _CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF );


Share this post


Link to post
Share on other sites
Quote:
Original post by _fastcall
Quote:
Original post by Kwizatz
Edit: Actually, the VC leak detection seems to be a pain to use


*** Source Snippet Removed ***


Exactly [smile], why can't it be a compiler flag?

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