• 13
• 18
• 19
• 27
• 10

# [C++]Memory leak

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

## Recommended Posts

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

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 on other sites
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 on other sites
Quote:
 Original post by 3DgonewildThere'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 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();  }  }

Thanks again.

##### Share on other sites
Quote:
 Original post by 3DgonewildThanks 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 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 on other sites
WOW , just WOW.
Thanks guys! i was sure that my code had some bugs!

##### 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 on other sites
Quote:
 Original post by 3DgonewildHmm..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.