From 63fffadacc51fb7c4915361f0286682e08a55cb1 Mon Sep 17 00:00:00 2001 From: Thorbjørn Lindeijer Date: Mon, 7 Oct 2024 12:19:10 +0200 Subject: Avoid some needless pointer indirection * Don't use `PlayerRelation*` in `mRelations`, but just store the value. * Pass `std::vector` by reference instead of pointer. * Return player list in `PlayerRelationsManager::getPlayers` by value instead of pointer. Overall these changes simplify the code, making it less prone to errors. --- src/gui/setup_players.cpp | 29 +++++++-------- src/playerrelations.cpp | 91 +++++++++++++++++------------------------------ src/playerrelations.h | 11 +++--- 3 files changed, 49 insertions(+), 82 deletions(-) diff --git a/src/gui/setup_players.cpp b/src/gui/setup_players.cpp index 17e59845..ebe570bb 100644 --- a/src/gui/setup_players.cpp +++ b/src/gui/setup_players.cpp @@ -95,12 +95,11 @@ public: { freeWidgets(); delete mListModel; - delete mPlayers; } int getRows() const override { - return mPlayers->size(); + return mPlayers.size(); } int getColumns() const override @@ -127,11 +126,10 @@ public: freeWidgets(); - delete mPlayers; mPlayers = player_relations.getPlayers(); // set up widgets - for (const auto &name : *mPlayers) + for (const auto &name : mPlayers) { gcn::Widget *widget = new Label(name); mWidgets.push_back(widget); @@ -161,20 +159,19 @@ public: virtual void freeWidgets() { - delete mPlayers; - mPlayers = nullptr; + mPlayers.clear(); delete_all(mWidgets); mWidgets.clear(); } - std::string getPlayerAt(int index) const + const std::string &getPlayerAt(int index) const { - return (*mPlayers)[index]; + return mPlayers[index]; } protected: - std::vector *mPlayers = nullptr; + std::vector mPlayers; std::vector mWidgets; PlayerRelationListModel *mListModel; }; @@ -189,7 +186,7 @@ public: int getNumberOfElements() override { - return player_relations.getPlayerIgnoreStrategies()->size(); + return player_relations.getPlayerIgnoreStrategies().size(); } std::string getElementAt(int i) override @@ -197,7 +194,7 @@ public: if (i >= getNumberOfElements()) return _("???"); - return (*player_relations.getPlayerIgnoreStrategies())[i]->mDescription; + return player_relations.getPlayerIgnoreStrategies()[i]->mDescription; } }; @@ -308,9 +305,9 @@ void Setup_Players::reset() // options in player_relations instead of strategies to sidestep this. int selection = 0; for (unsigned int i = 0; - i < player_relations.getPlayerIgnoreStrategies()->size(); + i < player_relations.getPlayerIgnoreStrategies().size(); ++i) - if ((*player_relations.getPlayerIgnoreStrategies())[i] == + if (player_relations.getPlayerIgnoreStrategies()[i] == player_relations.getPlayerIgnoreStrategy()) { @@ -375,19 +372,17 @@ void Setup_Players::action(const gcn::ActionEvent &event) else if (event.getId() == ACTION_DELETE) { int player_index = mPlayerTable->getSelectedRow(); - if (player_index < 0) return; - std::string name = mPlayerTableModel->getPlayerAt(player_index); - + const std::string &name = mPlayerTableModel->getPlayerAt(player_index); player_relations.removePlayer(name); } else if (event.getId() == ACTION_STRATEGY) { PlayerIgnoreStrategy *s = - (*player_relations.getPlayerIgnoreStrategies())[ + player_relations.getPlayerIgnoreStrategies()[ mIgnoreActionChoicesBox->getSelected()]; player_relations.setPlayerIgnoreStrategy(s); diff --git a/src/playerrelations.cpp b/src/playerrelations.cpp index 65b10a8c..731092f3 100644 --- a/src/playerrelations.cpp +++ b/src/playerrelations.cpp @@ -36,23 +36,21 @@ #define RELATION "relation" // constant for xml serialisation // (De)serialisation class -class PlayerConfSerialiser : public ConfigurationListManager, - std::map *> +class PlayerConfSerialiser : public ConfigurationListManager, + std::map *> { - ConfigurationObject *writeConfigItem(std::pair value, - ConfigurationObject *cobj) override + ConfigurationObject *writeConfigItem(std::pair value, + ConfigurationObject *cobj) override { - if (!value.second) - return nullptr; cobj->setValue(NAME, value.first); - cobj->setValue(RELATION, toString(value.second->mRelation)); + cobj->setValue(RELATION, toString(value.second.mRelation)); return cobj; } - std::map * + std::map * readConfigItem(ConfigurationObject *cobj, - std::map *container) override + std::map *container) override { std::string name = cobj->getValue(NAME, ""); if (name.empty()) @@ -62,7 +60,7 @@ class PlayerConfSerialiser : public ConfigurationListManagergetValue(RELATION, PlayerRelation::NEUTRAL); - (*container)[name] = new PlayerRelation(static_cast(v)); + (*container)[name] = PlayerRelation(static_cast(v)); } // otherwise ignore the duplicate entry @@ -94,19 +92,12 @@ PlayerRelationsManager::PlayerRelationsManager() : PlayerRelationsManager::~PlayerRelationsManager() { delete_all(mIgnoreStrategies); - - for (auto &[_, relation] : mRelations) - delete relation; } void PlayerRelationsManager::clear() { - std::vector *names = getPlayers(); - for (const auto &name : *names) - { + for (const auto &name : getPlayers()) removePlayer(name); - } - delete names; } #define PERSIST_IGNORE_LIST "persistent-player-list" @@ -115,9 +106,9 @@ void PlayerRelationsManager::clear() int PlayerRelationsManager::getPlayerIgnoreStrategyIndex(const std::string &name) { - std::vector *strategies = getPlayerIgnoreStrategies(); - for (unsigned int i = 0; i < strategies->size(); i++) - if ((*strategies)[i]->mShortName == name) + std::vector &strategies = getPlayerIgnoreStrategies(); + for (unsigned int i = 0; i < strategies.size(); i++) + if (strategies[i]->mShortName == name) return i; return -1; @@ -132,11 +123,11 @@ void PlayerRelationsManager::load() std::string ignore_strategy_name = config.getValue(PLAYER_IGNORE_STRATEGY, DEFAULT_IGNORE_STRATEGY); int ignore_strategy_index = getPlayerIgnoreStrategyIndex(ignore_strategy_name); if (ignore_strategy_index >= 0) - setPlayerIgnoreStrategy((*getPlayerIgnoreStrategies())[ignore_strategy_index]); + setPlayerIgnoreStrategy(getPlayerIgnoreStrategies()[ignore_strategy_index]); - config.getList, - std::map *> - ("player", &(mRelations), &player_conf_serialiser); + config.getList, + std::map *> + ("player", &mRelations, &player_conf_serialiser); } @@ -150,9 +141,9 @@ void PlayerRelationsManager::init() void PlayerRelationsManager::store() { - config.setList::const_iterator, - std::pair, - std::map *> + config.setList::const_iterator, + std::pair, + std::map *> ("player", mRelations.begin(), mRelations.end(), &player_conf_serialiser); @@ -177,18 +168,16 @@ unsigned int PlayerRelationsManager::checkPermissionSilently( const std::string &playerName, unsigned int flags) { - PlayerRelation *r = nullptr; - auto it = mRelations.find(playerName); - if (it != mRelations.end()) - r = it->second; - if (!r) + if (it == mRelations.end()) return mDefaultPermissions & flags; + PlayerRelation &r = it->second; + unsigned int permissions = - PlayerRelation::RELATION_PERMISSIONS[r->mRelation]; + PlayerRelation::RELATION_PERMISSIONS[r.mRelation]; - switch (r->mRelation) + switch (r.mRelation) { case PlayerRelation::NEUTRAL: permissions = mDefaultPermissions; @@ -236,29 +225,18 @@ bool PlayerRelationsManager::hasPermission(const std::string &name, void PlayerRelationsManager::setRelation(const std::string &playerName, PlayerRelation::Relation relation) { - PlayerRelation *r = nullptr; - - auto it = - mRelations.find(playerName); - if (it != mRelations.end()) - r = it->second; - if (!r) - mRelations[playerName] = new PlayerRelation(relation); - else - r->mRelation = relation; - + mRelations[playerName] = PlayerRelation(relation); signalUpdate(playerName); } -std::vector *PlayerRelationsManager::getPlayers() const +std::vector PlayerRelationsManager::getPlayers() const { - auto *retval = new std::vector(); + std::vector retval; - for (const auto &relation : mRelations) - if (relation.second) - retval->push_back(relation.first); + for (const auto &[name, _] : mRelations) + retval.push_back(name); - sort(retval->begin(), retval->end()); + sort(retval.begin(), retval.end()); return retval; } @@ -268,10 +246,7 @@ void PlayerRelationsManager::removePlayer(const std::string &name) auto it = mRelations.find(name); if (it != mRelations.end()) { - delete it->second; - mRelations.erase(it); - signalUpdate(name); } } @@ -281,7 +256,7 @@ PlayerRelation::Relation PlayerRelationsManager::getRelation(const std::string & { auto it = mRelations.find(name); if (it != mRelations.end()) - return it->second->mRelation; + return it->second.mRelation; return PlayerRelation::NEUTRAL; } @@ -352,7 +327,7 @@ public: } }; -std::vector * +std::vector & PlayerRelationsManager::getPlayerIgnoreStrategies() { if (mIgnoreStrategies.empty()) @@ -361,7 +336,7 @@ PlayerRelationsManager::getPlayerIgnoreStrategies() mIgnoreStrategies.push_back(new PIS_dotdotdot()); mIgnoreStrategies.push_back(new PIS_blinkname()); } - return &mIgnoreStrategies; + return mIgnoreStrategies; } diff --git a/src/playerrelations.h b/src/playerrelations.h index a836a976..2a0426a1 100644 --- a/src/playerrelations.h +++ b/src/playerrelations.h @@ -56,7 +56,7 @@ struct PlayerRelation IGNORED = 3 }; - PlayerRelation(Relation relation); + explicit PlayerRelation(Relation relation = NEUTRAL); Relation mRelation; // bitmask for all of the above }; @@ -158,11 +158,8 @@ public: /** * Retrieves all known player ignore strategies. - * - * The player ignore strategies are allocated statically and must not be - * deleted. */ - std::vector *getPlayerIgnoreStrategies(); + std::vector &getPlayerIgnoreStrategies(); /** * Return the current player ignore strategy. @@ -195,7 +192,7 @@ public: * Retrieves a sorted vector of all players for which we have any relations * recorded. */ - std::vector *getPlayers() const; + std::vector getPlayers() const; /** * Removes all recorded player info. @@ -231,7 +228,7 @@ private: unsigned int mDefaultPermissions; PlayerIgnoreStrategy *mIgnoreStrategy; - std::map mRelations; + std::map mRelations; std::list mListeners; std::vector mIgnoreStrategies; }; -- cgit v1.2.3-70-g09d2