What is wrong with this code? Seems that the pointers suddenly point to null values

Question

This is a section of code from a simple game I'm making. This routines check each bullet or enemy in the array and removes them from the array if they are already past the screen. (This code uses SDL2, but I don't think it matters.)

void cleanEnemies(Enemy* a[], int* s)
{
    Enemy* e;
    for (int i = 0; i < *s; i++)
    {
        e = a[i];
        if (e->getX() < -75)
        {
            e->~Enemy();
            e = NULL;
            for (int j = i; j < *s; j++)
            {
                a[j] = a[j + 1];
            }
            (*s)--;
        }
    }
    printf("Enemy count: %i\n", *s);
}

void cleanMissiles(HomingMissile* a[], int* s)
{
    HomingMissile* m;
    for (int i = 0; i < *s; i++)
    {
        m = a[i];
        if (m->getX() < -75)
        {
            m->~HomingMissile();
            m = NULL;
            for (int j = i; j < *s; j++)
            {
                a[j] = a[j + 1];
            }
            (*s)--;
        }
    }
    printf("Missile count: %i\n", *s);
}

Running the program produces pointers that point to invalid memory locations. Can anyone please tell me what I'm doing wrong? Thanks.

Why do you explicitly call the destructor in `m->~HomingMissile();` instead of using `delete`? — BlackDwarf, Oct 07 '15 at 09:21
And ditto `e->~Enemy()`? Also specifying which pointers point to invalid memory locations would help. It's difficult to debug this without a standalone example. Perhaps the destructors have side effects? — abligh, Oct 07 '15 at 09:22
BTW: A first view on the code tells me, that nobody can read it in a view seconds. Only single char names with a some operators over it. Looks very dirty! — Klaus, Oct 07 '15 at 09:42
unless you're using [placement new](http://stackoverflow.com/q/222557/995714), you'll never want to call constructors and destructors directly — phuclv, Oct 07 '15 at 09:50
I already tried using std::vector but there when the number of enemies exceed a certain limit, around 5, the enemies seem to be allocated in memory (as in, the missiles still follew them around) but SDL would not display their graphic. Would uploading the source for the whole program help? It's a school project, and I'm getting pretty desperate XD — Kyle Alexander Buan, Oct 08 '15 at 02:33

abligh · Answer 1 · 2015-10-07T09:40:01.310

The reason for the problem is (I think) that the inner for loop goes beyond the bounds of the array.

void cleanMissiles(HomingMissile* a[], int* s)
{
    HomingMissile* m;
    // Suggests there are *s entries, so last entry is (*s)-1
    for (int i = 0; i < *s; i++)
    {
        m = a[i];
        if (m->getX() < -75)
        {
            m->~HomingMissile();
            m = NULL;
            // Last value of j is (*s) - 1
            for (int j = i; j < *s; j++)
            {
                // Here it copies into a[(*s)] -1 from [a(*s)] which is invalid
                a[j] = a[j + 1];
            }
            (*s)--;
        }
    }
    printf("Missile count: %i\n", *s);
}

A fixed version would be as follows (relying on the fact that the for loop will never execute if the loop condition is not satisfied on entry):

void cleanMissiles(HomingMissile* a[], int* s)
{
    HomingMissile* m;
    for (int i = 0; i < *s; i++)
    {
        m = a[i];
        if (m->getX() < -75)
        {
            m->~HomingMissile();
            m = NULL;
            for (int j = i + 1; j < *s; j++)
            {
                a[j - 1] = a[j];
            }
            (*s)--;
        }
    }
    printf("Missile count: %i\n", *s);
}

But rather than fix this, please make your life easier and switch to std::vector or perhaps better std::list, and calling delete (assuming you allocated with new) rather than a direct call to the destructor.

Don't suggest calling `delete` until you know for sure that the object to delete was allocated using `new`. It can be an item from a statically allocated array. — axiac, Oct 07 '15 at 09:33
@axiac he was calling the destructor directly before. I can't imagine that's likely to be right given the circumstances. And I was suggesting doing this with a move to `std::vector` which would make that less likely still. But I will modify the text. — abligh, Oct 07 '15 at 09:39
Using [`std::vector`](http://en.cppreference.com/w/cpp/container/vector) is, indeed, a better solution. — axiac, Oct 07 '15 at 09:45

score 0 · Answer 2 · answered Oct 07 '15 at 20:47

With std::vector<std::unique_ptr<T>>, it becomes somethings like:

void cleanEnemies(std::vector<std::unique_ptr<Enemy>>& enemies)
{
    auto it = std::remove_if(enemies.begin(), enemies.end(),
                             [](const auto& enemy) {
                                  return enemy->getX() < -75;
                             });
    enemies.erase(it, enemies.end());
    std::cout << "Enemy count: " << enemies.size() << std::endl;
}

You can even make a template to handle both Enemy and HomingMissile

template <typename T>
void cleanObjects(std::vector<std::unique_ptr<T>>& objects, const std::string& objectName)
{
    auto it = std::remove_if(objects.begin(), objects.end(),
                             [](const auto& object) {
                                  return object->getX() < -75;
                             });
    objects.erase(it, objects.end());
    std::cout << objectName << " count: " << objects.size() << std::endl;
}

then cleanEnemies/cleanMissiles can be rewritten as:

void cleanEnemies(std::vector<std::unique_ptr<Enemy>>& enemies)
{
    cleanObjects(enemies, "Enemy");
}

void cleanMissiles(std::vector<std::unique_ptr<HomingMissile>>& missiles)
{
    cleanObjects(missiles, "Missile");
}

What is wrong with this code? Seems that the pointers suddenly point to null values

2 Answers2