Refactor ability lists using utils::erase_if and std::remove_if

C++20 will add std::erase_if as a convenience wrapper around std::remove_if,
handling the subsequent call to std::vector::erase. This simplifies readability,
avoiding the need to either store the result of std::remove_if in a temporary
or to put the second argument of vector::erase after the body of a lambda.

This adds utils::erase_if, which does the same thing. The C++20 function returns
the number of elements removed, but as none of the callers need that I've not
implemented it in utils::erase_if.

This commit omits the refactor of attack_type::overwrite_special_checking,
because that function is being worked on in parallel by Newfrenchy. It'll be
simpler for either of us to update it later instead of creating merge conflicts.

src/actions/heal.cpp

@@ -30,6 +30,7 @@
 #include "units/abilities.hpp"
 #include "units/udisplay.hpp"
 #include "units/map.hpp"
+#include "utils/general.hpp"
 
 #include <list>
 #include <vector>
@@ -200,16 +201,12 @@ namespace {
 		// Check healing from other units.
 		unit_ability_list heal_list = patient.get_abilities("heals");
 		// Remove all healers not on this side (since they do not heal now).
-		unit_ability_list::iterator heal_it = heal_list.begin();
-		while ( heal_it != heal_list.end() ) {
-			unit_map::iterator healer = units.find(heal_it->teacher_loc);
+		utils::erase_if(heal_list, [&](const unit_ability& i) {
+			unit_map::iterator healer = units.find(i.teacher_loc);
 			assert(healer != units.end());
 
-			if ( healer->side() != side )
-				heal_it = heal_list.erase(heal_it);
-			else
-				++heal_it;
-		}
+			return healer->side() != side;
+		});
 
 		// Now we can get the aggregate healing amount.
 		unit_abilities::effect heal_effect(heal_list, 0);

src/units/abilities.cpp

@@ -230,13 +230,9 @@ unit_ability_list unit::get_abilities(const std::string& tag_name, const map_loc
 unit_ability_list unit::get_abilities_weapons(const std::string& tag_name, const map_location& loc, const_attack_ptr weapon, const_attack_ptr opp_weapon) const
 {
 	unit_ability_list res = get_abilities(tag_name, loc);
-	for(unit_ability_list::iterator i = res.begin(); i != res.end();) {
-		if((!ability_affects_weapon(*i->ability_cfg, weapon, false) || !ability_affects_weapon(*i->ability_cfg, opp_weapon, true))) {
-			i = res.erase(i);
-		} else {
-			++i;
-		}
-	}
+	utils::erase_if(res, [&](const unit_ability& i) {
+		return !ability_affects_weapon(*i.ability_cfg, weapon, false) || !ability_affects_weapon(*i.ability_cfg, opp_weapon, true);
+	});
 	return res;
 }
 
@@ -1247,30 +1243,18 @@ unit_ability_list attack_type::get_weapon_ability(const std::string& ability) co
 {
 	const map_location loc = self_ ? self_->get_location() : self_loc_;
 	unit_ability_list abil_list(loc);
-	unit_ability_list abil_other_list(loc);
 	if(self_) {
-		abil_list.append((*self_).get_abilities(ability, self_loc_));
-		for(unit_ability_list::iterator i = abil_list.begin(); i != abil_list.end();) {
-			if(!special_active(*i->ability_cfg, AFFECT_SELF, ability, "filter_student")) {
-				i = abil_list.erase(i);
-			} else {
-				++i;
-			}
-		}
+		abil_list.append_if((*self_).get_abilities(ability, self_loc_), [&](const unit_ability& i) {
+			return special_active(*i.ability_cfg, AFFECT_SELF, ability, "filter_student");
+		});
 	}
 
 	if(other_) {
-		abil_other_list.append((*other_).get_abilities(ability, other_loc_));
-		for(unit_ability_list::iterator i = abil_other_list.begin(); i != abil_other_list.end();) {
-			if(!special_active_impl(other_attack_, shared_from_this(), *i->ability_cfg, AFFECT_OTHER, ability, "filter_student")) {
-				i = abil_other_list.erase(i);
-			} else {
-				++i;
-			}
-		}
+		abil_list.append_if((*other_).get_abilities(ability, other_loc_), [&](const unit_ability& i) {
+			return special_active_impl(other_attack_, shared_from_this(), *i.ability_cfg, AFFECT_OTHER, ability, "filter_student");
+		});
 	}
 
-	abil_list.append(abil_other_list);
 	return abil_list;
 }
 

src/units/unit.cpp

@@ -1655,13 +1655,9 @@ int unit::resistance_against(const std::string& damage_name,bool attacker,const
 	int res = movement_type_.resistance_against(damage_name);
 
 	unit_ability_list resistance_abilities = get_abilities_weapons("resistance",loc, weapon, opp_weapon);
-	for(unit_ability_list::iterator i = resistance_abilities.begin(); i != resistance_abilities.end();) {
-		if(!resistance_filter_matches(*i->ability_cfg, attacker, damage_name, 100-res)) {
-			i = resistance_abilities.erase(i);
-		} else {
-			++i;
-		}
-	}
+	utils::erase_if(resistance_abilities, [&](const unit_ability& i) {
+		return !resistance_filter_matches(*i.ability_cfg, attacker, damage_name, 100-res);
+	});
 
 	if(!resistance_abilities.empty()) {
 		unit_abilities::effect resist_effect(resistance_abilities, 100-res);

src/units/unit.hpp

@@ -96,16 +96,29 @@ public:
 	const unit_ability& back()  const  { return cfgs_.back();  }
 
 	iterator erase(const iterator& erase_it)  { return cfgs_.erase(erase_it); }
+	iterator erase(const iterator& first, const iterator& last)  { return cfgs_.erase(first, last); }
 
 	template<typename... T>
 	void emplace_back(T&&... args) { cfgs_.emplace_back(args...); }
 
 	const map_location& loc() const { return loc_; }
 
-	/** Appens the abilities from @a other to @a this, ignores other.loc() */
+	/** Appends the abilities from @a other to @a this, ignores other.loc() */
 	void append(const unit_ability_list& other)
 	{
-		std::copy( other.begin(), other.end(), std::back_inserter(cfgs_ ));
+		std::copy(other.begin(), other.end(), std::back_inserter(cfgs_ ));
+	}
+
+	/**
+	 * Appends any abilities from @a other for which the given condition returns true to @a this, ignores other.loc().
+	 *
+	 * @param other where to copy the elements from
+	 * @param predicate a single-argument function that takes a reference to an element and returns a bool
+	 */
+	template<typename Predicate>
+	void append_if(const unit_ability_list& other, const Predicate& predicate)
+	{
+		std::copy_if(other.begin(), other.end(), std::back_inserter(cfgs_ ), predicate);
 	}
 
 private:

src/utils/general.hpp

@@ -94,4 +94,16 @@ inline bool contains(const Container& container, const Value& value)
  */
 std::string get_unknown_exception_type();
 
+/**
+ * Convenience wrapper for using std::remove_if on a container.
+ *
+ * todoc++20 use C++20's std::erase_if instead. The C++20 function returns the number of elements
+ * removed; this one could do that but it seems unnecessary to add it unless something is using it.
+ */
+template<typename Container, typename Predicate>
+void erase_if(Container& container, const Predicate& predicate)
+{
+	container.erase(std::remove_if(container.begin(), container.end(), predicate), container.end());
+}
+
 } // namespace utils