C++ how to erase from vector while iterating

Question

std::vector<int *>intList;
int *a = new int;
*a = 3;
int *b = new int;
*b = 2;
int *c = new int;
*c = 1;

intList.push_back(a);
intList.push_back(b);
intList.push_back(c);

std::vector<int *>::iterator it;

for (it = intList.begin(); it != intList.end();)
{
    int *a = (int*)*it;

    if ((*a) == 2)
    {
        delete a;
        intList.erase(it);
    }
    else
    {
        ++it;
    }
}

This code crashes at the start of the for loop when something is erased from the vector. I do not understand why. I am using Visual Studio 2015 if that helps

Answer 1

erase returns next iterator.

if ((*a) == 2)
{
    delete a;
    it = intList.erase(it);
}

EDIT: remove() and remove_if() will copy the elements(pointer here) and one will end up with multiple elements pointing to same integer and if you then try to free them, you'll be left with dangling pointers.

Consider the vector has 4 elements which look something like

0x196c160 0x196bec0 0x196c180 0x196bee0 

One might be tempted to use erase-remove idiom

auto temp = remove_if(vec.begin(),vec.end(),[](const auto &i){return *i==2;});

Now it looks like

0x144aec0 0x144b180 0x144b180 0x144aee0

temp would be pointing to 3rd element and a

for(auto it=temp;it!=vec.end();it++)
    delete *it;

Now the second element is a dangling pointer.

EDIT 2: The above problem could be solved if you delete before the element is copied.Look at @Dietmar's answer.

Answer 2

Better to use std::vector<std::unique_ptr<int>> (or even std::vector<int> if you don't need pointer).

then just use erase-remove idiom:

std::vector<int> intList{3, 2, 1};

intList.erase(std::remove(intList.begin(), intList.end(), 2), intList.end());

or

std::vector<std::unique_ptr<int>> intList;
intList.puch_back(std::make_unique<int>(3));
intList.puch_back(std::make_unique<int>(2));
intList.puch_back(std::make_unique<int>(1));

intList.erase(std::remove_if(intList.begin(), intList.end(),
                             [](const auto& p){ return *p == 2; }),
              intList.end());

If you really need raw owning pointer, you may use a variant using partition:

std::vector<int*> intList{ new int {3}, new int {2}, new int{1} };

auto it = std::partition(intList.begin(), intList.end(),
                         [](const auto& p){ return *p != 2; });

std::foreach (it, intList.end(), [](int* p) { delete p; });
intList.erase(it, intList.end());

Finally, if you really need to do it manually, you have to fix your erase line to:

it = intList.erase(it);

to have:

std::vector<int*> intList{ new int {3}, new int {2}, new int{1} };

for (auto it = intList.begin(); it != intList.end(); /*Empty*/) {
    int *p = *it;

    if (*p == 2) {
        delete p;
        it = intList.erase(it);
    } else {
        ++it;
    }
}