C++ Clearing vector of pointers to dynamically-allocated object causes exception?

Question

I have a class called ClassA. This ClassA holds a vector of pointers to dynamically allocated objects of class ClassB. The destructor for ClassA looks like this:

ClassA::~ClassA() {
    for (vector<ClassB*>::iterator i = m_vActiveB.begin(), e = m_vActiveB.end(); i != e; ) {
        vector<ClassB*>::iterator tmp(i++);
        delete *tmp;
        m_vActiveB.erase(tmp);
    }
}

In ClassC, I have an vector of ClassA objects. My current scenario is this: if I don't have any ClassA objects in my ClassC vector, then it all works fine. But if I have objects in there, and then call .erase() on my vector in ClassC, then an exception is thrown. Through logging I have determined that the ClassA destructor is called (as it should be, as my understanding is that .erase() destroys each vector member), but the exception is thrown when the loop begins.

Does anyone know what might be causing this? Thanks!

Answer 1

Follow the rule of zero and just use the following to declare your m_vActiveB member vector:

std::vector<ClassB> m_vActiveB;

and let the default constructor take care of destruction. You could alternatively use a vector of std::unique_ptr, but, just like your current solution, you would not take advantage of the natural efficiency of contiguous elements in arrays.

In my system objects of ClassB themselves need to be constantly, added, removed and updated from the vector to keep a state model.

If your data model doesn't naturally base on the fact that elements are contiguous and have a particular order, an "unsorted", associative container like std::unordered_map or std::unordered_set might be a better solution here, especially if you insert and remove the elements that aren't residing at the end of the vector (and sit in the middle instead).

You might also want to take a look at std::list and std::deque which offer, respectively, fast insertion and deletion (which don't invalidate iterators) and fast insertion and deletion at both ends (which don't invalidate iterators either).

I recall that altering the vector contents invalidates all pointers.. is that correct?

If you call std::vector::erase yes, all iterators will get invalidated (including the past the end iterator). Calling erase will also make all references and pointers to the deleted object invalid (just like referencing any object that is destroyed/deallocated is invalid).

Answer 2

From the docs: http://en.cppreference.com/w/cpp/container/vector/erase

Invalidates iterators and references at or after the point of the erase, including the end() iterator.

This means your i and e iterators have both been invalidated. So don't use erase inside a loop like you're doing!

Also consider:

1. Using smart pointers, like std::shared_ptr instead of doing memory management yourself.
2. Looking up the erase-remove, though it isn't quite so useful here because:
3. You're deleting every element in the vector. You don't need to erase them as you go along, just use m_vActiveB.clear() when you've deleted them all, but you don't even need to do that because:
4. This is a destructor and m_vActiveB is a member. It'll get tidied up for you, don't bother clearing it yourself.

Answer 3

Using std::vector::erase causes iterators at and past the point of erase to become invalidated. As such happens, the past-the-end iterator e is no longer valid after the first iteration.

Instead compare directly against the iterator returned from std::vector::end at every iteration. However, in your example there's no need to erase anything as m_vActiveB is a member and will be destructed automatically after running the dtor (deleteing the data pointed to is however required).

The iterator i will also become invalidated when the erase happens as making a copy tmp of the iterator i and erasing it will still cause the same element to be erased from the vector (erasing a copy of an iterator does not help).

ClassA::~ClassA() {
    for (vector<ClassB*>::iterator i = m_vActiveB.begin(); i != m_vActiveB.end(); ++i) {
        //vector<ClassB*>::iterator tmp(i++); // Unnecessary
        delete *i;
        //m_vActiveB.erase(i); // Unnecessary as elements will be erased automatically.
    }
}

Equivalent C++11 code:

ClassA::~ClassA() {
    for (auto ptr : m_vActiveB) {
        delete ptr;
    }
}

A better alternative would be to use a C++11 smart pointer as e.g. std::unique_ptr, that can handle the allocation and deallocation automatically.

class ClassA {
    std::vector<std::unique_ptr<ClassB>> m_vActiveB; // No need for explicit destructor.
    /* ... */
};

Or if you are not using polymorphism or have some other specific reason to use pointers, then why use pointers at all? Simply use a std:vector<ClassB>.

Answer 4

You are removing elements from the vector while iterating through it. That invalidates the iterators.

It should be something like this:

ClassA::~ClassA() 
{
    for (auto e : m_vActiveB)
    {
        delete e;
    }
}

You don't have to clear the vector, it is destroyed anyway after the destructor has finished.

Answer 5

When you use vector::erase, it invalidates any other iterator into the vector.

So you cannot use a "traditional" iterator loop.

The simplest way is as Rakibul Hasan suggests, just delete all pointers and then clear the vector.