4

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.

Brian Tompsett - 汤莱恩
  • 5,753
  • 72
  • 57
  • 129
James Richmond
  • 55
  • 1
  • 1
  • 7

6 Answers6

6

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.

Community
  • 1
  • 1
Mike Seymour
  • 249,747
  • 28
  • 448
  • 644
  • This doesn't seem to work either. I did try something like this before but nothing seemes to work. It's as if it doesn't like the delete command. stupid question: I assume I don't need to include a certain header to delete a pointer. – James Richmond Sep 02 '13 at 13:56
  • @RustyC: The `delete` command is fine, *if* it's a valid pointer. Are you accidentally copying the vector and deleting the objects twice? Is `GameState` a base class, with different types for the actual objects? If so, make sure it has a virtual destructor. If not, why are you storing pointers in the first place? – Mike Seymour Sep 02 '13 at 13:58
  • Gamestate holds virtual calls for init, render, update and close but each child of it would also hold a number of their own functions and variables. Since I only want one running at a time the manager calls init when the state is active and close when its done. The destructor is essentially empty and there is a line to close the active state directly above the line I quoted. I expect these classes to be quite large so I was trying to save space by storing them on the heap. would they still need a virtual destructor in the base class if they would be essentially empty. – James Richmond Sep 02 '13 at 14:03
  • @RustyC: Yes, you always need a virtual destructor to delete via a base-class pointer. Make sure that `GameState` has one, and also make sure that the manager class isn't accidentally copying the vector. – Mike Seymour Sep 02 '13 at 14:04
  • the vector is defined (private to the class) in the header and added to using push_back, I can't see how it would be copied at any point. Also, would a virtual destructor need anything partcicular in it? – James Richmond Sep 02 '13 at 14:09
  • @RustyC: It could be copied if you accidentally copy the entire manager class; you should make sure that that class follows the Rule of Three and is either not copyable, or correctly copyable. Using `unique_ptr` would automatically prevent copying for you. The virtual destructor can be empty if there's nothing to be done to clean up the base class; it just needs to exist. – Mike Seymour Sep 02 '13 at 14:11
  • It seems the problem lies with not haveing a virtual destructor. I'll probally change the manager to a singleton to prevent copying or possibly use unique_ptr instead. It's just nice to not have it crash when it closes for now, thanks for your help. – James Richmond Sep 02 '13 at 14:13
  • @RustyC: Singletons are generally regarded as a bad idea, especially in C++. Just prevent copying by deleting the copy constructor and assignment operator: `Manager(Manager const&) = delete; void operator=(Manager const&) = delete;` – Mike Seymour Sep 02 '13 at 14:16
2

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.

Mats Petersson
  • 126,704
  • 14
  • 140
  • 227
0

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

Marius Bancila
  • 16,053
  • 9
  • 49
  • 91
0

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

erase already advances for you.

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

Yakk - Adam Nevraumont
  • 262,606
  • 27
  • 330
  • 524
0

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.

Useless
  • 64,155
  • 6
  • 88
  • 132
  • "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." Unless it's not, in which case it won't! – Daniel Earwicker Sep 02 '13 at 13:56
  • Question says this is in the manager class' destructor, and I'm assuming the vector is a member sub-object. – Useless Sep 02 '13 at 14:01
0

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.

CygnusX1
  • 20,968
  • 5
  • 65
  • 109