How do I delete an object pointer from a vector without causing a memory error?

Question

I have a vector of object pointers that I am adding to and deleting from while looping through to update objects. I can't seem to delete objects that have "died" from the vector without causing a memory error. I'm not really sure what I'm doing wrong. Listed below is my update method and it's sub method.

void Engine::update(string command){
    if(getGameOver()==false){
        for(p=objects.begin();p!=objects.end();p++){
        spawnUpdate(command);
        //cout<<"Spawn"<<endl;
        objectUpdate(command);
        //cout<<"object"<<endl;
        scrollUpdate(command);
    //  cout<<"scroll"<<endl;
        killUpdate(command);
        //cout<<"kill"<<endl;
}
}
}

void Engine::killUpdate(std::string command){
    if((*p)->getIsDead()==true){delete *p;}
}

void Engine::objectUpdate(string command){
    (*p)->update(command,getNumObjects(),getObjects());
    if(((*p)->getType() == PLAYER)&&((*p)->getPosX()>=getFinishLine())){setGameOver(true);}
}

void Engine::scrollUpdate(string command){
            //Check player position relative to finishLine
            if(((*p)->getType() == PLAYER)&&((*p)->getPosX()>(SCREEN_WIDTH/2))){
                (*p)->setPosX((*p)->getPosX()-RUN_SPEED);
                setFinishLine(getFinishLine()-RUN_SPEED);

                for(q=objects.begin();q!=objects.end();q++){
                    //Shift objects to pan the screen
                    if((*q)->getType() == OPPONENT){(*q)->setPosX((*q)->getPosX()-RUN_SPEED);}
                    if((*q)->getType() == BLOCK){(*q)->setPosX((*q)->getPosX()-RUN_SPEED);}
                }
            }
}

void Engine::spawnUpdate(string command){
    if(command.compare("shoot")==0){
        cout<<"Bang!"<<endl;
            if((*p)->getType() == PLAYER){objects.push_back(new Bullet((*p)->getPosX(),(*p)->getPosY(),(*p)->getState()));cout<<"Bullet success "<<endl;}
        }
}

You need to remove the deleted ones from the `std::vector` otherwise the next time round you will be testing unassigned memory. Otherwise you could set them to `nullptr` and check for `nullptr` before using them. (Or remove all the `nullptr` elements from the `std::vector` after processing it). — Galik, Dec 06 '14 at 05:40
First thing is to name your variables more descriptively than `p` and `q`. Second, format your code a little better, as the code is hard to read. — PaulMcKenzie, Dec 06 '14 at 05:40
The problem is that you're changing your vector's size while looping over your vector. This will invalidate iterators -- don't write code that changes the vector's size while your looping over the same vector -- the problem is much more than a `delete` call. — PaulMcKenzie, Dec 06 '14 at 05:44
How would I be able to work around this? The update method is continuously called while the game is not over, and I would need to iterate through the vector to know what objects would need to be removed. — Tanner Marshall, Dec 06 '14 at 06:00
One way to manage this is while processing the `std::vector` add new elements to a separate `std::vector` and mark elements to be deleted with `nullptr`. Then after you finish processing the whole `std::vector`, make the changes by erasing the `nullptr` elements and appending the new elements from the other vector. (or replacing the `nullprt` elements from the new `std::vector` and appending/erasing the difference). — Galik, Dec 06 '14 at 06:03
I guess you have `p` as a class member variable, that is an **extremely bad idea**. You should be passing `p` or `*p` to any function that requires it, instead of `command` which is not even being used! — M.M, Aug 28 '15 at 00:35

score 2 · Answer 1 · edited May 23 '17 at 11:58

Some assumptions/definitions:

objects a member variable, something like vector<Object*> objects;
p is also a member variable, something like vector<Object*>::iterator p;

So p is an iterator, *p is an Object pointer, and **p is an Object.

The problem is that this method:

void Engine::killUpdate(std::string command) {
  if ((*p)->getIsDead() == true) {
    delete *p;
  }
}

deallocates the Object pointed to by *p, the pointer in the vector at the position referenced by the p iterator. However the pointer *p itself is still in the vector, now it just points to memory that is no longer allocated. Next time you try to use this pointer, you will cause undefined behavior and very likely crash.

So you need to remove this pointer from your vector once you have deleted the object that it points to. This could be as simple as:

void Engine::killUpdate(std::string command) {
  if ((*p)->getIsDead() == true) {
    delete *p;
    objects.erase(p);
  }
}

However, you are calling killUpdate from update in a loop that iterates over the objects vector. If you use the code above, you will have another problem: once you erase p from the objects vector, it is no longer safe to execute p++ in your for-loop statement, because p is no longer a valid iterator.

Fortunately, STL provides a very nice way around this. vector::erase returns the next valid iterator after the one you erased! So you can have the killUpdate method update p instead of your for-loop statement, e.g.

void Engine::update(string command) {
  if (getGameOver() == false) {
    for (p = objects.begin(); p != objects.end(); /* NOTHING HERE */) {
      // ...
      killUpdate(command);
    }
  }
}

void Engine::killUpdate(std::string command) {
  if ((*p)->getIsDead() == true) {
    delete *p;
    p = objects.erase(p);
  } else {
    p++;
  }
}

This is of course assuming that you always call killUpdate in the loop, but I'm sure you can see the way around this if you don't -- just execute p++ at the end of the for-loop body in the case that you haven't called killUpdate.

Also note that this is not particularly efficient, since every time you erase an element of the vector, the elements that follow it have to be shifted back to fill in the empty space. So this will be slow if your objects vector is large. If you used a std::list instead (or if you are already using that), then this is not a problem, but lists have other drawbacks.

A secondary approach is to overwrite each pointer to a deleted object with nullptr and then use std::remove_if to remove them all in one go at the end of the loop. E.g.:

void Engine::update(string command) {
  if (getGameOver() == false) {
    for (p = objects.begin(); p != objects.end(); p++) {
      // ...
      killUpdate(command);
    }
  }
  std::erase(std::remove_if(objects.begin(), objects.end(), 
                            [](const Object* o) { return o == nullptr; }), 
             objects.end());
}

void Engine::killUpdate(std::string command) {
  if ((*p)->getIsDead() == true) {
    delete *p;
    *p = nullptr;
  } 
}

The assumption this time is that you will never have a nullptr element of objects that you want to keep for some reason.

Since you seem to be a beginner, I should note that this:

  std::erase(std::remove_if(objects.begin(), objects.end(), 
                            [](const Object* o) { return o == nullptr; }),
             objects.end());

is the erase-remove idiom, which is explained well on Wikipedia. It erases elements from the vector if they return true when a given function object is called on them. In this case, the function object is:

[](const Object* o) { return o == nullptr; }

Which is a lambda expression and is essentially shorthand for an instance of an object with this type:

class IsNull {
 public:
   bool operator() (const Object* o) const {
     return o == nullptr;
   }
};

One last caveat to the second approach, I just noticed that you have another loop over objects in scrollUpdate. If you choose the second approach, be sure to update this loop to check for nullptrs in objects and skip them.

When attempting to implement the second option it states that remove_if is not a member of namespace std. — Tanner Marshall, Dec 06 '14 at 07:18
After a bit of searching I figured out that it needs #include . I think I still have something wrong because I still receive memory errors. — Tanner Marshall, Dec 06 '14 at 07:27

score 0 · Answer 2 · answered Dec 06 '14 at 06:21

Here is an issue (formatted for readability):

void Engine::update(string command)
{
    if (getGameOver()==false)
    {
        for (p=objects.begin();p!=objects.end();p++)
        {
            spawnUpdate(command);  // changes vector
//...
       }
    }
//...
}

void Engine::spawnUpdate(string command)
{
//...
    objects.push_back(new Bullet((*p)->getPosX(),(*p)->getPosY(),(*p)->getState())); // no
//...
}

You have a loop with iterator p that points to elements in the object vector. When you call objects.push_back, the iterator for the vector may become invalidated. Thus that loop iterator p is no longer any good. Incrementing it in the for() will cause undefined behavior.

One way to get around this is to create a temporary vector that holds your updates. Then you add the updates at the end of your processing:

void Engine::update(string command)
{
    std::vector<Object*> subVector;
    if (getGameOver()==false)
    {
        for (p=objects.begin();p!=objects.end();p++)
        {
            spawnUpdate(command, subVector);
            //...
        }
    }

    // add elements to the vector
    object.insert(object.end(), subVector.begin(), subVector.end());
}

void Engine::spawnUpdate(string command, std::vector<Object*>& subV)
{
    if (command.compare("shoot")==0)
    {
        cout<<"Bang!"<<endl;
        if ((*p)->getType() == PLAYER)
            subV.push_back(new Bullet((*p)->getPosX(),(*p)->getPosY(),(*p)->getState()));
        cout<<"Bullet success "<<endl;
    }
}

score 0 · Answer 3 · answered Aug 28 '15 at 00:52

You could avoid most of these issues by not using raw pointers. Clearly your code uses the semantic that the vector owns the pointers, so you can express this directly:

std::vector< std::unique_ptr<Object> > objects;

Then you may insert into the vector by using objects.emplace_back(arguments,to,Object,constructor); , and when you remove from the vector it will automatically delete the Object.

You still need to watch out for erase invalidating iterators, so keep using the erase-remove idiom as explained by Tyler McHenry. For example:

objects.erase( std::remove_if( begin(objects), end(objects),
    [](auto&& o) { return o->getIsDead(); }),  end(objects) );

Note - auto&& is permitted here since C++14; in C++11 you'd have to use std::unique_ptr<Object>&. Required includes are <algorithm> and <memory>.

And please stop using global iterators, keep p local to the function and pass any arguments you need to pass.

How do I delete an object pointer from a vector without causing a memory error?

3 Answers3