Memory leaks from removing elements from list.

Started by
15 comments, last by iMalc 10 years, 10 months ago

It gave me compiling errors until I changed it to:


lifeforms.remove_if([](lifeform* n){ return n->food < 1; } );
I'll change to a regular list if it's a problem.
But will that cause a noticable drop in preformance?
(note that I have thousands of objects in a list each having their functions run and values modifyed and accessed per frame)

Sorry about the typo, oops. :)

For the most part, you should end up with better performance given the way you are using the list. Individual operations are a little more expensive but that is pretty much trivial and can be ignored. You should not be worrying about the performance at this level, it is a minor issue compared to what your other functions are likely going to be doing.

Advertisement
I switched my code to use a regular list, and it's working alright.
But I guess I have a slightly different problem now: memory leaks.
I kinda figured when elements were removed from the list they weren't freed from memory, so I tried something like this:
if((*it)->food < 1)
{
     for( auto leaf = (*it)->cells.begin(); leaf!=(*it)->cells.end(); ++leaf )
          free(*leaf);
     void *old = *it;
     it = lifeforms.erase(it);
     free(old);
}
I guess how this works depends on the actual class, so I'll post what I'm using right now:
class cell;

class lifeform
{
    public:
    list<cell*> cells;
    int x,y;
    int food;
    int age;
    int pointerX,pointerY;
    int data[geneMaxLines][geneMaxArgs];

    void doFunction(int a,int b,int c,int d,int e);
    bool addCell(int newx,int newy,int newr,int newg,int newb);
};

list<lifeform*> lifeforms;

class cell
{
    public:
    int x,y;
    bool alive;
    int type;
    int r,g,b;
    int food;
    Uint32 color;
    lifeform *parent;
    int eat();
    void show();
};
This seems to cut down on the memory leaks, however I get a strage problem that the number of elements in the list is drastically differnet from the lists size, once the programs been running for a bit and many elements have been removed:
int cellCount = 0;
for( auto it = lifeforms.begin(); it!=lifeforms.end(); ++it )
      cellCount++;
int difference = ((signed int)lifeforms.size())-cellCount;
Difference fluctuates rapidly, itll be in the thousands one frame and then tens the next.

Note that if I go back to just inserting the:
lifeforms.remove_if([](lifeform* n){ return n->food < 1; } );
the difference stops doing that.
Memory leaks occur with both methods, if I don't include free statements.

Why do you have a list of pointers to cells anyway, why not just a list of cells?

Why are you using free? You must 'delete' what you 'new'. As this is clearly C++ code, you should not be using malloc or free.

Why do you have a void pointer? Those are very very seldom useful in C++. Call delete through a pointer to the right type, or base class type.

Those are the obvious problems to fix first.

"In order to understand recursion, you must first understand recursion."
My website dedicated to sorting algorithms

Why do you have a list of pointers to cells anyway, why not just a list of cells?

Read the OP.
That was the origional problem.
I needed to modify the elements of the list and run functions on them.
If I used pointers then I'd just get a COPY of the object, and the modifications would not stick.

Why are you using free? You must 'delete' what you 'new'. As this is clearly C++ code, you should not be using malloc or free.

Crashes the program.
I try "delete &old;" after the erase and it doesn't work.
I've tried a few other things as well.

Why do you have a void pointer? Those are very very seldom useful in C++. Call delete through a pointer to the right type, or base class type.

Doesn't seem to make a difference.

Those are the obvious problems to fix first.

Can't really fix problems concerning how "common" or "correct" something is if the program doesn't even stay open.
At least with the things I was doing it was working, it just had a huge memory leak.

Why do you have a list of pointers to cells anyway, why not just a list of cells?

Read the OP.
That was the origional problem.
I needed to modify the elements of the list and run functions on them.
If I used pointers then I'd just get a COPY of the object, and the modifications would not stick.

Why are you using free? You must 'delete' what you 'new'. As this is clearly C++ code, you should not be using malloc or free.

Crashes the program.
I try "delete &old;" after the erase and it doesn't work.
I've tried a few other things as well.

Why do you have a void pointer? Those are very very seldom useful in C++. Call delete through a pointer to the right type, or base class type.

Doesn't seem to make a difference.

Those are the obvious problems to fix first.

Can't really fix problems concerning how "common" or "correct" something is if the program doesn't even stay open.
At least with the things I was doing it was working, it just had a huge memory leak.

Oh now I see what you meant earlier. You were in fact mistaken right from your very first post. You can of course modify items in a list, and no I certainly don't mean "replace". You can make whatever changes to the items in a list that you like. Same for a vector. This is good news because it means you probably can forget about storing pointers to objects afterall, and have the containers manage the memory for you.

Post the code for whatever "tests" you did if you want to find out what led to your incorrect conclusion.

You are using delete wrong. You can't delete the address of a local or other variable, you must pass just the pointer varible itself to delete.

There are on if's or but's about it either. If you allocated using 'new' then you MUST destroy using 'delete', end of story.

Deleting through a pointer to the correct type or base type will indeed make a very big difference. It will mean that it actually works, instead of causing memory leaks and all sorts of other nasties like you are having.

These are not merely suggestions, like telling you that you could replace a division with a shift or something trivial and unimportant like that. These are vital corrections. You will not get a correctly functioning program without doing so, and you will not fix your memory leaks.

Failing to match new and delete, or new[] and delete[], is precisely what is probably causing the leaks to begin with.

If you don't understand the advice, then ask more questions, or post code so that we can see what you are doing wrong.

"In order to understand recursion, you must first understand recursion."
My website dedicated to sorting algorithms
If you want to modify a value in a container, don't copy the dereferenced iterator into another value. Either use the arrow operator, or bind the dereferenced iterator to a reference:

for (auto it = c.begin() ; it != c.end() ; ++i) {
    it-> frobnicate();
    // Or ...
    Example &example = *it;
    example.foo();
    example.bar();
}

Good point, I should have noticed he was doing that in the original post.

"In order to understand recursion, you must first understand recursion."
My website dedicated to sorting algorithms

This topic is closed to new replies.

Advertisement