Why is looping over a container and modifying it considered bad practice?

I would like to understand why it is considered bad practice to do this. I do get the "desired" output when printing the vector. (1,2,4,5).

int main()
{
    std::vector<int> numbers = {1, 2, 3, 4, 5};

    for (auto it = numbers.begin(); it != numbers.end(); ++it)
    {
        // Accessing invalidated iterator leads to undefined behavior
        if (*it == 3)
        {
            numbers.erase(it); // Invalidates the iterator
        }
    }

    for_each(numbers.begin(), numbers.end(),([](int x){cout << x << '\n';}));
}

I know I could also do:

numbers.erase(std::remove(numbers.begin(), numbers.end(), 3), numbers.end());

By I want to understand why the first approach can lead to undefined behavior and what is happening underneath.

numbers.erase(it) will invalidate the iterator it, it can no longer be used for anything. It no longer "point" to an element that exists in the vector. The iterator has been "disconnected" from the vector.

You should instead use the iterator that the erase function returns as the "next" iterator (and skip ++it).

For a simple example of what can happen, think about the case where you remove the last element from the vector. You then increase the iterator to point to.... Where? Increasing the iterator will not make it point to the end iterator, and since it's "disconnected" from the vector it used to belong to, there's really no way to ell exactly what or where it might "point" to.

So to continue with the example of erasing the last element, you will do an invalid increase, and invalid comparison which will likely be true (since the invalid iterator will not be the same as the vector end iterator) and the loop will continue forever.

But even just erasing an element in the middle will invalidate the iterator, so no matter for which element you call erase, using the same iterator again leads to undefined behavior (which is an error, and not only a bad practice).

For completeness, here's the loop as it should be:

for (auto it = numbers.begin(); it != numbers.end(); /* Empty */)
{
    if (*it == 3)
    {
        // Erase the element, and get an iterator to the element after the erased
        // Or if it's the last element then get the end iterator
        it = numbers.erase(it);
    }
    else
    {
        // Not erasing element, increase iterator to point to the next element
        ++it;
    }
}

This is an example of what makes Undefined Behavior so insidious and difficult to deal with -- your code may appear to work fine when in fact it is broken and when you change something apparently unrelated, all hell will break loose, making it very hard to figure out where the actual problem is.

Because std::vector iterators are (usually) implemented as pointers into the vector, an operation like erase that invalidates the iterator will still leave the iterator as an apparently valid pointer, it just points at the wrong place. So if you use it, it is easy for it to appear to work, though subtle (or not so subtle) problems may appear later on. For your specific case, look at what happens if your input vector is something like:

std::vector<int> numbers = {1, 2, 3, 3, 4, 5};

in this case, after erasing the first 3, the iterator will point at the second 3, and then the loop increment will go on to the 4, so the second 3 won't be erased.

"Why is looping over a container and modifying it considered bad practice?" - Because modifying the container potentially invalidates iterators and (potentially) indices. So the iteration suddenly doesn't iterate over what it thought it was iterating over and chaos ensues.