Jump to content
  • Advertisement
Sign in to follow this  
3Dgonewild

[C++]Memory leak

This topic is 4098 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

There's a bug in my program...memory leak i think. Here's my code:
 std::vector<mOBJECT>  DATABASE;

//add an object..
     mOBJECT *tmp=new mOBJECT(x,y,VSIZE); 
    DATABASE.push_back(*tmp);


And where it crashes:
void releaselist(std::vector<mOBJECT>& l)
{
for (int i=0;i<l.size();l++) 
{
delete &l.at(i); //<-----HERE IT CRASHES..
}
}



I've tried to replace the above function with this code:
//MOBJECT CLASS
~mOBJECT()
{
delete this;
}

--in main() :
while()
{
}..
for (int i=0;i<DATABASE.size();i++)
{
DATABASE.at(i).~mOBJECT();
}

But this freezes my program on startup.. What im doing wrong?

Share this post


Link to post
Share on other sites
Advertisement
Why are you initializing your objects using pointers, then pushing the plain objects onto the vector anyway?

When you copy the mOBJECT pointed to by tmp, you are not dealing with a pointer anymore, so there is no reason to delete the contents of the vector. Furthermore, once you push the object onto the vector, you need to delete tmp to prevent memory leaks.

Alternatively, you can store a vector of mOBJECT pointers and delete them when they are removed.

Share this post


Link to post
Share on other sites
Quote:
Original post by 3Dgonewild
There's a bug in my program...memory leak i think.

Here's my code:

*** Source Snippet Removed ***
And where it crashes:

*** Source Snippet Removed ***
I've tried to replace the above function with this code:

*** Source Snippet Removed ***
But this freezes my program on startup..

What im doing wrong?


What are you doing wrong? You don't understand how std::vector works yet is all. [smile]

std::vector manages its own memory. In this case, you create a vector of mOBJECT types. When you push_back a mOBJECT instance, the vector stores a copy of it.

So what you should be doing is:

void foo()
{
std::vector<mOBJECT> DATABASE;

// push an anonymous mOBJECT into the vector
DATABASE.push_back(mOBJECT(x,y,VSIZE));

// no need to do anything, std::vector cleans up
}






Note:

//MOBJECT CLASS
~mOBJECT()
{
delete this;
}

--in main() :
while()
{
}..
for (int i=0;i<DATABASE.size();i++)
{
DATABASE.at(i).~mOBJECT();
}





This is wrong, there is almost never a good reason to manually call the destructor of an object. There are occasions where objects can commit suicide using "delete this;", but most of the time there are better alternatives. You should certainly not be calling it in the destructor (part of deletes job is to call the destructor, so your program likely goes into an infinite loop).

This is all assuming that mOBJECT isn't intended to be used as a base class in an inheritance hierarchy. If that is the case, you need to store either pointers (hard) or smart pointers to mOBJECT derived classes in your vector. This time when you push_back a pointer, the vector again stores a copy - but only a copy of the pointer. This means that you will have to manually clear the objects the pointers in the vector point at.

Share this post


Link to post
Share on other sites
Thanks guys!
Just one more question.

Is it bad if i remove objects like this??:


if (!DATABASE.empty()) {
for ( int x=0;x<DATABASE.size();x++)
{
if (DATABASE.at(x).x>=SCREEN_WIDTH)
{
vector<mOBJECT>::iterator temp;
temp=DATABASE.begin()+x;
DATABASE.erase(temp);
cout<<"Item "<<x<<" deleted!"<<endl;
continue;//<--without this crashes..
}
DATABASE.at(x).render();
}
}

I need your opinion.

Thanks again.

Share this post


Link to post
Share on other sites
Quote:
Original post by 3Dgonewild
Thanks guys!
Just one more question.

Is it bad if i remove objects like this??:

*** Source Snippet Removed ***
I need your opinion.

Thanks again.


Well, I would use std::remove_if with the appropriate functor:


struct OffScreen {
bool operator()( const mOBJECT &object )
{
return object.x >= SCREEN_WIDTH;
}
};

void process( std::vector<mOBJECT> &DATABASE ).
{
// remove_if partitions the vector in 2 based on a predicate
// basically the iterate now points at all the objects we want to remove
std::vector<mOBJECT>::iterator it = std::remove_if(DATABASE.begin(),DATABASE.end(),OffScreen());
// remove all off screen entities
DATABASE.erase(it,DATABASE.end());
// draw the rest
std::for_each(DATABASE.begin(),DATABASE.end(),std::mem_fun_ref(&mOBJECT::render) );
}




Your code crashes as you modify the vector while iterating over it. If you need to do this (I prefer keeping the w 2 operations separate where possible) then do all the iteration using iterators, like so:


for( std::vector<mOBJECT>::iterator it = DATABASE.begin() ; it != DATABASE.end() ; ) {
if( it->x > SCREEN_WIDTH ) {
it = DATABASE.erase(it);
} else {
it->render();
++it;
}
}



Note that the above could be less efficient as each erase will involve a load of unnecessary copy operations.



Let us discuss your naming convention.

This isn't a huge deal while you are programming on your own, but if you want to work with other people it will. Firstly, why "mOBJECT" over simply "OBJECT". What does the "m" stand for?

Also, you already use all capitals for your constant SCREEN_WIDTH. This is idiomatic in C++. Not so for variables. Why can't "DATABASE" be just "database"? Likewise, types in C++ are usually not in all capitals. A common (but by no means universal) preference is camel case. Personally, I use lowerCamelCase for variables and UpperCamelCase for types.

However your choice of naming convention is not important, it is more important to be consistent. Your code is not consistent as "temp" is a variable and it does not appear to follow your naming convention.

Share this post


Link to post
Share on other sites
You should consider using an iterator instead of an index:


if (!DATABASE.empty())
{
for ( vector<mOBJECT>::iterator x=DATABASE.begin();x!=DATABASE.end();)
{
if (x->x>=SCREEN_WIDTH)
{
x = DATABASE.erase(x);
cout<<"Item "<<x - DATABASE.begin()<<" deleted!"<<endl;
}
else
{
x->render();
++x;
}
}
}

Share this post


Link to post
Share on other sites
Hmm..i have another question...

I've edited the offscreen struct a little:

struct OffScreen {
private:

public:
bool operator()( const mOBJECT &object )
{
if((object.x >= SCREEN_WIDTH+100)||(object.x<=-100)||(object.y>=SCREEN_HEIGHT+100)||(object.y<=-100))
{
cout<<"Object '"<<object.id<<"' deleted!"<<endl;
return true;
}

return false;

}
};


The problem is that the log remains empty!!!!!!!!!!

Share this post


Link to post
Share on other sites
Quote:
Original post by 3Dgonewild
Hmm..i have another question...

I've edited the offscreen struct a little:
*** Source Snippet Removed ***
The problem is that the log remains empty!!!!!!!!!!


Personally, I would move that output statement to the objects destructor, because that is when the object is actually deleted.

Other than that, we could apply the "simplest solution" rule here. The simplest solution, I dare to suggest, is that no objects are falling outside of the screen bounds you have defined. Are you 100% sure that some objects are doing so? Another possibility is that the calling code, the remove_if function, is not being executed for some reason. Again, are you 100% sure that the calling code is itself being executed.

If both of these are occuring, we will probably need to see more code.

Share this post


Link to post
Share on other sites
Sorry for the trouble , i found out what was wrong.
There was a little problem with particle class(actually,i hadn't update the scroll function with the new bounds)..

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.

GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry.

Sign me up!