How to properly delete 1+ object from vector of uniq_ptr's

Question

I have:

        std::vector<std::unique_ptr<Zombie>> zombie;
        for(unsigned j=0; j < zombie.size(); j++)
        {    
              if(bullet intersects zombie)
              {             
                  zombie.erase(zombie.begin()+j)
              }
        }

I would like to delete specific object/s in vector of unique pointers. Above example works, but my lecturer said that this way doesn't guarantee that all proper objects will be deleted. I have spent long time to figure it out somehow, but I don't have idea how to do it.

What if there are two zombies right next to each other that are to be erased? Check your logic, especially with `j`. Or imagine if there are only two zombies in the vector, and both are to be removed. How will your loop work? (again, check `j`). — PaulMcKenzie, Jul 15 '20 at 17:21
Understanding the problem with the current code is a requirement in order to understand *what* you need to fix. And to understand what is wrong now, think about how both `j` and `zombie` look like after your code execute the `if` branch — UnholySheep, Jul 15 '20 at 17:21
This has nothing to do, whatsoever, with `uniq_ptr`s per se. The overall removal logic is flawed, and has the same problem no matter what's in the vector. — Sam Varshavchik, Jul 15 '20 at 17:36

Remy Lebeau · Accepted Answer · 2020-07-15T20:24:03.887

The lecturer is correct.

Your loop unconditionally increments j on every iteration regardless of whether erase() is called. So, whenever you do call erase(), the indexes of the remaining elements decrement, but you increment j anyway, skipping the next element that followed the "erased" element.

One solution is to increment j only on iterations that do not call erase(), eg:

std::vector<std::unique_ptr<Zombie>> zombies;
...
for(size_t j = 0; j < zombies.size(); )
{    
    auto &zombie = zombies[j];

    if (bullet intersects zombie)
    {
        zombies.erase(zombies.begin()+j);
    }
    else
    {
        ++j;
    }
}

Or, using iterators instead of indexes:

std::vector<std::unique_ptr<Zombie>> zombies;
...
for(auto iter = zombies.begin(); iter != zombies.end(); )
{    
    auto &zombie = *iter;

    if (bullet intersects zombie)
    {
        iter = zombies.erase(iter);
    }
    else
    {
        ++iter;
    }
}

An alternative solution is to use the remove-erase idiom instead, eg:

std::vector<std::unique_ptr<Zombie>> zombies;
...
zombies.erase(
    std::remove_if(zombies.begin(), zombies.end(),
        [&](std::unique_ptr<Zombie> &zombie) { return (bullet intersects zombie); }
    ),
    zombies.end()
);

std::remove_if() will first "remove" all of the matching elements by moving them to the end of the vector, returning an iterator to the first "removed" element. And then passing that range of "removed" elements to std::vector::erase() will physically remove them from the vector.

Alternatively, in C++20 and later, you can use std::erase_if() instead, which makes the remove_if() and erase() calls for you, eg:

std::vector<std::unique_ptr<Zombie>> zombies;
...
std::erase_if(zombies,
    [&](auto &zombie){ return (bullet intersects zombie); }
);

How to properly delete 1+ object from vector of uniq_ptr's

1 Answers1