CODEWITHSUNDEEP
c++c++11visual-c++
Kookies
Kookies

Reputation: 73

C++ object deletion crashing

I tried to include all necessary code that may be causing these problems. The program crashes the second time this code is run:

        for (int i = 0; i < player.projectiles.size(); i++)
        {
            bool temp = player.projectiles[i].move(begin, WIDTH);
            if (temp == true)
            {
                player.projectiles[i].destroy();
                player.projectiles.erase(player.projectiles.begin() + i);
            }
        }

Player.projectiles is a vector of objects from the class Projectiles. The member function move works perfectly and the function destroy looks like this:

void Projectile::destroy()
{
    delete this;
}

The first time the loop is run everything works fine, but the second time my program crashes and I can't figure out why. I suspect it has to do with deleting the object. Any help is greatly appreciated.

Upvotes: 0

Views: 236

Answers (1)

Remy Lebeau
Remy Lebeau

Reputation: 598134

You are not storing Projectile* pointers in your vector, you are storing actual Projectile objects (as evident by the fact that your loop is accessing members of the Projectile class using operator. instead of operator->). So, calling destroy() on any of those objects is absolutely the wrong thing to do, because you don't own that memory! The vector does, and as such will destroy each object for you when they are removed from the vector, including when the vector is cleared, or destroyed itself.

That being said, you are also modifying player.projectiles while iterating through it, so your loop WILL, at the very least, skip elements as elements are being erased, and could even go out of bounds. You MUST NOT increment the i counter on iterations where erase() is called, eg:

for (size_t i = 0; i < player.projectiles.size(); )
{
    if (player.projectiles[i].move(begin, WIDTH))
        player.projectiles.erase(player.projectiles.begin() + i);
    else
        ++i;
}

Alternatively, vector::erase() returns an iterator to the next element following the one being erased, so have your loop use iterators instead of indexes, eg:

for(auto iter = player.projectiles.begin(); iter != player.projectiles.end(); )
{
    if (iter->move(begin, WIDTH))
        iter = player.projectiles.erase(iter);
    else
        ++iter;
}

Alternatively, you can replace the whole loop by using the erase-remove idiom via std::remove_if(), eg:

#include <algorithm>

player.projectiles.erase(
    std::remove_if(player.projectiles.begin(), player.projectiles.end(),
        [=](auto &projectile) { return projectile.move(begin, WIDTH); }
    ),
    player.projectiles.end()
);

Or, std::erase_if() in C++20:

std::erase_if(
    player.projectiles,
    [=](auto &projectile) { return projectile.move(begin, WIDTH); }
);

Upvotes: 3

Related Questions