Game Events: throw out custom handler_list implementation

This replaces it with the standard std::list interface. I've attempted to make this change as least invasive
as possible, so I have not touched the manager::iteration class. It can probably be refactored further later,
but the crux of this change is simply to allow the removal of the custom smart_list container while keeping
the chance of breakage to a minimum.

The new implementation cannot be an std::forward_list since that only has front-insertion capabilities
while back-insertion is needed.

src/game_events/handlers.cpp

@@ -45,29 +45,6 @@ static lg::log_domain log_event_handler("event_handler");
 // This file is in the game_events namespace.
 namespace game_events
 {
-/* ** handler_list::iterator ** */
-
-/**
- * Dereference.
- * If the current element has become invalid, we will increment first.
- */
-handler_ptr handler_list::iterator::operator*()
-{
-	// Check for an available handler.
-	while(iter_.derefable()) {
-		// Handler still accessible?
-		if(handler_ptr lock = iter_->lock()) {
-			return lock;
-		} else {
-			// Remove the now-defunct entry.
-			iter_ = list_t::erase(iter_);
-		}
-	}
-
-	// End of the list.
-	return handler_ptr();
-}
-
 /* ** event_handler ** */
 
 event_handler::event_handler(const config& cfg, bool imi, handler_vec::size_type index, manager& man)

src/game_events/handlers.hpp

@@ -24,8 +24,8 @@
 #pragma once
 
 #include "config.hpp"
-#include "utils/smart_list.hpp"
 
+#include <list>
 #include <memory>
 #include <set>
 #include <string>
@@ -80,104 +80,6 @@ class event_handler
 	config cfg_;
 };
 
-/**
- * This is a wrapper for a list of weak pointers to handlers. It allows forward
- * iterations of the list, with each element returned as a shared pointer.
- * (Weak pointers that fail to lock are silently removed from the list.) These
- * iterations can be used recursively, even when the innermost iteration might
- * erase arbitrary elements from the list.
- *
- * The interface is not the standard list interface because that would be
- * inconvenient. The functionality implemented is that required by Wesnoth.
- */
-class handler_list
-{
-	/// The weak pointers that are used internally.
-	typedef std::weak_ptr<event_handler> internal_ptr;
-
-	/// The underlying list.
-	typedef utils::smart_list<internal_ptr> list_t;
-
-public:
-	/**
-	 * Handler list iterators are rather limited. They can be constructed
-	 * from a reference iterator (not default constructed), incremented,
-	 * and dereferenced. Consecutive dereferences are not guaranteed to
-	 * return the same element (if the list mutates between them, the next
-	 * element might be returned). An increment guarantees that the next
-	 * dereference will differ from the previous (unless at the end of the
-	 * list). The end of the list is indicated by dereferencing to a null
-	 * pointer.
-	 */
-	class iterator
-	{
-		/// The current element.
-		list_t::iterator iter_;
-
-	public:
-		/// Initialized constructor (to be called by handler_list).
-		explicit iterator(const list_t::iterator& base_iter)
-			: iter_(base_iter)
-		{
-		}
-
-		/// Increment.
-		iterator& operator++()
-		{
-			++iter_;
-			return *this;
-		}
-		/// Dereference.
-		handler_ptr operator*();
-	};
-	friend class iterator;
-	typedef iterator const_iterator;
-
-public:
-	/**
-	 * Default constructor.
-	 * Note: This explicit definition is required (by the more pedantic
-	 *       compilers) in order to declare a default-constructed, static,
-	 *       and const variable in event_handlers::get(), in handlers.cpp.
-	 */
-	handler_list()
-		: data_()
-	{
-	}
-
-	const_iterator begin() const
-	{
-		return iterator(const_cast<list_t&>(data_).begin());
-	}
-
-	// The above const_cast is so the iterator can remove obsolete entries.
-	const_iterator end() const
-	{
-		return iterator(const_cast<list_t&>(data_).end());
-	}
-
-	// push_front() is probably unneeded, but I'll leave the code here, just in case.
-	// (These lists must be maintained in index order, which means pushing to the back.)
-	void push_front(const handler_ptr& p)
-	{
-		data_.push_front(internal_ptr(p));
-	}
-
-	void push_back(const handler_ptr& p)
-	{
-		data_.push_back(internal_ptr(p));
-	}
-
-	void clear()
-	{
-		data_.clear();
-	}
-
-private:
-	/// No implementation of operator=() since smart_list does not support it.
-	handler_list& operator=(const handler_list&);
-
-	/// The actual list.
-	list_t data_;
-};
+using weak_handler_ptr = std::weak_ptr<event_handler>;
+using handler_list = std::list<weak_handler_ptr>;
 }

src/game_events/manager.cpp

@@ -108,12 +108,21 @@ manager::iteration::iteration(const std::string& event_name, manager& man)
 	, var_it_(event_name.empty() ? var_list_.end() : var_list_.begin())
 	, gamedata_(resources::gamedata)
 {
+	// Clean up expired ptrs. This saves us effort later since it ensures every ptr is valid.
+
+	main_list_.remove_if(
+		[](weak_handler_ptr ptr) { return ptr.expired(); }
+	);
+
+	var_list_.remove_if(
+		[](weak_handler_ptr ptr) { return ptr.expired(); }
+	);
 }
 
 /**
  * Increment
  * Incrementing guarantees that the next dereference will differ from the
- * previous derference (unless the iteration is exhausted). However, multiple
+ * previous dereference (unless the iteration is exhausted). However, multiple
  * increments between dereferences are allowed to have the same effect as a
  * single increment.
  */
@@ -139,22 +148,32 @@ manager::iteration& manager::iteration::operator++()
 	return *this;
 }
 
+// Small helper function to ensure we don't try to dereference an invalid iterator.
+static handler_ptr lock_ptr(const handler_list& list, handler_list::iterator iter)
+{
+	if(iter != list.end()) {
+		return iter->lock();
+	}
+
+	return nullptr;
+}
+
 /**
  * Dereference
  * Will return a null pointer when the end of the iteration is reached.
  */
 handler_ptr manager::iteration::operator*()
 {
 	// Get the candidate for the current element from the main list.
-	handler_ptr main_ptr = *main_it_;
+	handler_ptr main_ptr = lock_ptr(main_list_, main_it_);
 	handler_vec::size_type main_index = ptr_index(main_ptr);
 
 	// Get the candidate for the current element from the var list.
-	handler_ptr var_ptr = *var_it_;
+	handler_ptr var_ptr = lock_ptr(var_list_, var_it_);
 
 	// (Loop while var_ptr would be chosen over main_ptr, but the name does not match.)
 	while(var_ptr && var_ptr->index() < main_index && !var_ptr->matches_name(event_name_, gamedata_)) {
-		var_ptr = *++var_it_;
+		var_ptr = lock_ptr(var_list_, ++var_it_);
 	}
 
 	handler_vec::size_type var_index = ptr_index(var_ptr);

src/game_events/manager.hpp

@@ -78,9 +78,9 @@ class manager
 
 	private:
 		/// The fixed-name event handlers for this iteration.
-		const handler_list& main_list_;
+		handler_list& main_list_;
 		/// The varying-name event handlers for this iteration.
-		const handler_list& var_list_;
+		handler_list& var_list_;
 		/// The event name for this iteration.
 		const std::string event_name_;
 		/// The end of this iteration. We intentionally exclude handlers

src/game_events/manager_impl.cpp

@@ -80,10 +80,10 @@ std::string event_handlers::standardize_name(const std::string& name)
 /**
  * Read-only access to the handlers with fixed event names, by event name.
  */
-const handler_list& event_handlers::get(const std::string& name) const
+handler_list& event_handlers::get(const std::string& name)
 {
 	// Empty list for the "not found" case.
-	static const handler_list empty_list;
+	static handler_list empty_list;
 
 	// Look for the name in the name map.
 	auto find_it = by_name_.find(standardize_name(name));

src/game_events/manager_impl.hpp

@@ -26,7 +26,7 @@ class event_handlers
 {
 private:
 	typedef std::unordered_map<std::string, handler_list> map_t;
-	typedef std::unordered_map<std::string, std::weak_ptr<event_handler>> id_map_t;
+	typedef std::unordered_map<std::string, weak_handler_ptr> id_map_t;
 
 	/**
 	 * Active event handlers. Will not have elements removed unless the event_handlers is clear()ed.
@@ -61,8 +61,8 @@ class event_handlers
 	{
 	}
 
-	/** Read-only access to the handlers with varying event names. */
-	const handler_list& get_dynamic() const
+	/** Access to the handlers with varying event names. */
+	handler_list& get_dynamic()
 	{
 		return dynamic_;
 	}
@@ -73,8 +73,8 @@ class event_handlers
 		return active_;
 	}
 
-	/** Read-only access to the handlers with fixed event names, by event name. */
-	const handler_list& get(const std::string& name) const;
+	/** Access to the handlers with fixed event names, by event name. */
+	handler_list& get(const std::string& name);
 
 	/** Adds an event handler. */
 	void add_event_handler(const config& cfg, manager& man, bool is_menu_item = false);