The obvious problem: You are using an invalid iterator. When you delete an element from a std::list, iterators pointing at other elements remain valid, but the deleted element’s iterator is in an undefined state. A possible solution is to iterate one step ahead of deletions:
template <typename List>
void ConsumeList(List&& list) {
auto last_it = list.begin();
if (last_it != list.end()) {
for (auto it = std::next(last_it); it != list.end(); ++it) {
list.erase(last_it); // last_it becomes invalid here.
last_it = it; // Never mind, we make it valid again.
}
list.erase(last_it); // Don't forget this!
}
}
Other notes, mostly unrelated to the problem:
- Avoid methods such as
Destroy(); it is likely doing something that ~GameMatch() should be doing instead, but without strong RAII-related guarantees from the compiler. Prefer RAII over “homebrew” resource management.
- Do not use raw pointers. You almost never(*) need them.
(*) The only exception is when you implement low-level linked data structures. Then (and only then) it makes sense to use raw pointers.
- Preferably, do not allocate objects directly (using smart pointers or otherwise) at all. Instead, let STL containers handle allocation. Embed your objects directly (
std::list<GameMatch> instead of std::list<GameMatch*>). The fewer pointer jumps, the better efficiency you get.
Here’s ConsumeList() once again, this time in a buildable and runnable context:
#include <iostream>
#include <iterator>
#include <list>
#include <memory>
#include <string>
#include <string_view>
#include <utility>
namespace {
struct GameMatch {
GameMatch(std::string_view match_id) : match_id_{match_id} {}
std::string_view GetMatchId() const { return match_id_; }
~GameMatch() { std::cout << "Destoying " << GetMatchId() << std::endl; }
private:
const std::string match_id_;
};
template <typename List>
void ConsumeList(List&& list) {
auto last_it = list.begin();
if (last_it != list.end()) {
for (auto it = std::next(last_it); it != list.end(); ++it) {
list.erase(last_it);
last_it = it;
}
list.erase(last_it);
}
}
} // namespace
int main() {
std::list<GameMatch> embedded_list;
embedded_list.emplace_back("Embedded Match A");
embedded_list.emplace_back("Embedded Match B");
embedded_list.emplace_back("Embedded Match C");
std::list<std::unique_ptr<GameMatch>> pointer_list;
pointer_list.emplace_back(std::make_unique<GameMatch>("Allocated Match A"));
pointer_list.emplace_back(std::make_unique<GameMatch>("Allocated Match B"));
pointer_list.emplace_back(std::make_unique<GameMatch>("Allocated Match C"));
ConsumeList(std::move(embedded_list));
ConsumeList(std::move(pointer_list));
}
Notice that forgetting to run ConsumeList() will not cause any memory leaks at the end and all list elements will be properly deallocated. That is the key advantage of using RAII and avoiding raw pointers.
