Avoid some needless pointer indirection

* Don't use `PlayerRelation*` in `mRelations`, but just store the value.

* Pass `std::vector<PlayerIgnoreStrategy *>` 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.

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<std::string> *mPlayers = nullptr;
+    std::vector<std::string> mPlayers;
     std::vector<gcn::Widget *> 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::pair<std::string, PlayerRelation *>,
-                                                             std::map<std::string, PlayerRelation *> *>
+class PlayerConfSerialiser : public ConfigurationListManager<std::pair<std::string, PlayerRelation>,
+                                                             std::map<std::string, PlayerRelation> *>
 {
-    ConfigurationObject *writeConfigItem(std::pair<std::string, PlayerRelation *> value,
-                                                 ConfigurationObject *cobj) override
+    ConfigurationObject *writeConfigItem(std::pair<std::string, PlayerRelation> 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::string, PlayerRelation *> *
+    std::map<std::string, PlayerRelation> *
     readConfigItem(ConfigurationObject *cobj,
-                   std::map<std::string, PlayerRelation *> *container) override
+                   std::map<std::string, PlayerRelation> *container) override
     {
         std::string name = cobj->getValue(NAME, "");
         if (name.empty())
@@ -62,7 +60,7 @@ class PlayerConfSerialiser : public ConfigurationListManager<std::pair<std::stri
         if (it != (*container).end())
         {
             int v = (int)cobj->getValue(RELATION, PlayerRelation::NEUTRAL);
-            (*container)[name] = new PlayerRelation(static_cast<PlayerRelation::Relation>(v));
+            (*container)[name] = PlayerRelation(static_cast<PlayerRelation::Relation>(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<std::string> *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<PlayerIgnoreStrategy *> *strategies = getPlayerIgnoreStrategies();
-    for (unsigned int i = 0; i < strategies->size(); i++)
-        if ((*strategies)[i]->mShortName == name)
+    std::vector<PlayerIgnoreStrategy *> &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::pair<std::string, PlayerRelation *>,
-                   std::map<std::string, PlayerRelation *> *>
-        ("player",  &(mRelations), &player_conf_serialiser);
+    config.getList<std::pair<std::string, PlayerRelation>,
+                   std::map<std::string, PlayerRelation> *>
+        ("player",  &mRelations, &player_conf_serialiser);
 }
 
 
@@ -150,9 +141,9 @@ void PlayerRelationsManager::init()
 
 void PlayerRelationsManager::store()
 {
-    config.setList<std::map<std::string, PlayerRelation *>::const_iterator,
-                   std::pair<std::string, PlayerRelation *>,
-                   std::map<std::string, PlayerRelation *> *>
+    config.setList<std::map<std::string, PlayerRelation>::const_iterator,
+                   std::pair<std::string, PlayerRelation>,
+                   std::map<std::string, PlayerRelation> *>
         ("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<std::string> *PlayerRelationsManager::getPlayers() const
+std::vector<std::string> PlayerRelationsManager::getPlayers() const
 {
-    auto *retval = new std::vector<std::string>();
+    std::vector<std::string> 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<PlayerIgnoreStrategy *> *
+std::vector<PlayerIgnoreStrategy *> &
 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<PlayerIgnoreStrategy *> *getPlayerIgnoreStrategies();
+    std::vector<PlayerIgnoreStrategy *> &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<std::string> *getPlayers() const;
+    std::vector<std::string> getPlayers() const;
 
     /**
      * Removes all recorded player info.
@@ -231,7 +228,7 @@ private:
     unsigned int mDefaultPermissions;
 
     PlayerIgnoreStrategy *mIgnoreStrategy;
-    std::map<std::string, PlayerRelation *> mRelations;
+    std::map<std::string, PlayerRelation> mRelations;
     std::list<PlayerRelationsListener *> mListeners;
     std::vector<PlayerIgnoreStrategy *> mIgnoreStrategies;
 };