From 67f2c32c54e65055abc909e219a87c65ad11ea14 Mon Sep 17 00:00:00 2001 From: Steve Cotton Date: Fri, 18 Nov 2022 17:40:02 +0100 Subject: [PATCH] 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 | 13 +++++-------- src/units/abilities.cpp | 34 +++++++++------------------------- src/units/unit.cpp | 10 +++------- src/units/unit.hpp | 17 +++++++++++++++-- src/utils/general.hpp | 12 ++++++++++++ 5 files changed, 44 insertions(+), 42 deletions(-) diff --git a/src/actions/heal.cpp b/src/actions/heal.cpp index 060bfb72898..568c4e85277 100644 --- a/src/actions/heal.cpp +++ b/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 #include @@ -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); diff --git a/src/units/abilities.cpp b/src/units/abilities.cpp index f5d865c51ed..d0eaa151cf1 100644 --- a/src/units/abilities.cpp +++ b/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; } diff --git a/src/units/unit.cpp b/src/units/unit.cpp index b9f98dd236e..e2d7da87ee2 100644 --- a/src/units/unit.cpp +++ b/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); diff --git a/src/units/unit.hpp b/src/units/unit.hpp index be0b5d73fc2..a9fff04faa5 100644 --- a/src/units/unit.hpp +++ b/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 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 + void append_if(const unit_ability_list& other, const Predicate& predicate) + { + std::copy_if(other.begin(), other.end(), std::back_inserter(cfgs_ ), predicate); } private: diff --git a/src/utils/general.hpp b/src/utils/general.hpp index d20b2efe720..e90d2d0d205 100644 --- a/src/utils/general.hpp +++ b/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 +void erase_if(Container& container, const Predicate& predicate) +{ + container.erase(std::remove_if(container.begin(), container.end(), predicate), container.end()); +} + } // namespace utils