CODEWITHSUNDEEP
c++pointersvectordelete-operator
James Richmond
James Richmond

Reputation: 55

Having trouble deleting vector of pointers

I have a manager class holding a vector of pointers to a virtual base class to allow a variety of child classes to be stored there. In the destructor of this manager class, I want it to cycle through all the pointers it holds and delete them. However, I have tried a number of methods I've come across and the program keeps crashing during the execution.

The current code I have is shown below:-

for (std::vector<GameState*>::iterator it = gamestates_.begin(); it != gamestates_.end(); ++it){
    delete *it;
    it = gamestates_.erase(it);
}

One thing I haven't tried yet is using unique_ptr but I'm sure this should be able to handle it without using them. Please correct me if I'm wrong.

Edit: I'm aware I should clear the vector after the loop but this is what I've come to after trying every normal method of deleting the pointers. It doesn't seem to like the delete command.

Upvotes: 4

Views: 16483

Answers (6)

Mike Seymour
Mike Seymour

Reputation: 254431

Erasing an element from the vector invalidates the iterator, so you can't continue iterating afterwards. In this case, I wouldn't erase elements within the loop; I'd clear the vector afterwards:

for (auto it = gamestates_.begin(); it != gamestates_.end(); ++it){
    delete *it;
}
gamestates_.clear();

Although, if this is in the destructor and the vector is about to be destroyed, there's no point clearing it either.

If you do need to erase within a loop (perhaps because you only want to erase some elements), then you need a bit more care to keep the iterator valid:

for (auto it = gamestates_.begin(); it != gamestates_.end();){ // No ++ here
    if (should_erase(it)) {
        it = gamestates_.erase(it);
    } else {
        ++it;
    }
}

One thing I haven't tried yet is using unique_ptr but I'm sure this should be able to handle it without using them. Please correct me if I'm wrong.

If you do want to manage dynamic objects by steam by this, then make sure you follow the Rule of Three: you'll need to implement (or delete) the copy constructor and copy-assignment operator to prevent "shallow" copying leaving you with two vectors that try to delete the same objects. You'll also need to take care to delete the objects in any other place that removes or replaces them. Storing smart pointers (or the objects themselves, if you don't need pointers for polymorphism) will take care of all these things for you, so I would always recommend that.

I'm aware I should clear the vector after the loop but this is what I've come to after trying every normal method of deleting the pointers. It doesn't seem to like the delete command.

The most likely cause is that you're not following the Rule of Three, and are accidentally trying to delete the same objects twice after copying the vector. It's also possible that GameState is a base class and you forgot to give it a virtual destructor, or that the pointers have been corrupted by some other code.

Upvotes: 6

CygnusX1
CygnusX1

Reputation: 21779

The problem is that you are incrementing it twice. First, when you call it = .erase(it) which returns the next element, and then in the loop ++i. You may inadvertely jump over the end and things may go wrong, not to mention that you will remove only every second element of your vector.

A simple fix is to just not change it in the loop (no ++it).

A better way would be to actually delete the array from the end of the vector, as erasing elements from inside the vector introduces an expensive move of all its follow-up elements. Your current algorithm will work in N^2 time. Try something like this:

while (!gamestates_.empty()) {
    delete gamestates_.back();
    gamestates_.erase(gamestates_.end()-1);
}

You may also just iterate over all elements of the vector and clear it afterwards:

for (std::vector<GameState*>::iterator it = gamestates_.begin(); it != gamestates_.end(); ++it){
    delete *it;
}
gamestates_.clear();

Also note, that the clear() operation of the vector is also done in its destructor. If the deletion procedure is part of some destruction process where gamestates_ is ulimately destroyed - you don't have to call clear() at all.

Upvotes: 0

Useless
Useless

Reputation: 67713

Prefer to use unique_ptr. You say you should be able to handle it without using them as if getting a smart pointer to do the work for you is some kind of terrible imposition.

They're there to make your life easier, you don't have to feel guilty about not doing the hard work by hand.

And with your existing code, just don't call erase. The vector is going to be destroyed anyway, right? It'll take care of all that itself.

Upvotes: 0

Yakk - Adam Nevraumont
Yakk - Adam Nevraumont

Reputation: 275260

get rid of the ++it in your for loop header.

erase already advances for you.

Alternatively, iterate, delete, then after iteration .clear().

Upvotes: 0

Mats Petersson
Mats Petersson

Reputation: 129314

Your iterator is updated twice in each loop: 

it = gamestates_.erase(it);

and 

it++

You only need the first one - it already points at the "next object" in the container.

Upvotes: 2

Marius Bancila
Marius Bancila

Reputation: 16318

Erasing an element from your vector invalidates the iterators. Delete the objects pointer by the elements and then clear() the content of the vector.

Upvotes: 0

Related Questions