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;}
        }
}

Answer 1

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.

Answer 2

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.

Answer 3

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;
    }
}