So the problem is that when i try to push non-dynamic obj to playerList or when I try to delete n I get segfault (core dump). I assume that the problem is caused when Helper class is being destroyed so the vector also is being destroyed so it tries to destroy object in itself which does not exist anymore. However when i use playerList.clear() the problem still exist. I think i could just destroy objects in playerList() with ~Helper(). But I would like to know why i cannot use non-dynamic objects and just clear them out of playerList at the end of Run().
class Helper{
public:
void Run();
private:
std::vector<Player>playerList;
...
};
that's how Run() looks like:
using namespace std;
void Helper::Run(){
Player *n = new Player();
playerList.push_back(*n); //Yup. There is a memleak
}
also
Player.h:
class Player{
public:
...
~Player();
private:
...
IClass* typeOfClass = new Warrior();
};
and ~Player:
Player::~Player(){
delete typeOfClass;
}
and Warrior (has no effect on the problem)
class Warrior {
public:
int GetMeleeAttack();
int GetRangedAttack();
int GetMagicAttack();
int AgilityAction();
int StrengthAction();
int IntelligenceAction();
void WhoAmI();
private:
};
Warrior's methods just returns some integers.
std::vector<Player>playerList;
should be
std::vector<Player*>playerList;
if you want to allocate them dynamically. The other method would be to emplace each element and not use new.
When using new you are allocating on the heap but you are creating a new element in the vector by passing the value from the one allocated on heap. And you have a dangling pointer ( memory leak )
Remember to deallocate all the elements at the destruction of the vector if you are using a vector of pointers.
Another method would be:
std::vector<std::unique_ptr<Player> >playerList;
That will take care of the allocation issue.
A std::vectors main job is to dynamically allocate objects for you, so you don't need to use new and delete.
void Helper::Run(){
playerList.push_back(Player());
}
This will default-construct a new player object and add it to the vector.
Solution
Yes, the problem can be solved by storing pointers to Player, but this needlessly adds to the problems caused by having to store a pointer to IClass inside Player.
Instead you can make the whole problem go away by making
IClass* typeOfClass;
into
std::unique_ptr<IClass> typeOfClass;
or
std::shared_ptr<IClass> typeOfClass;
and using
playerList.emplace_back();
in Helper::Run.
TL;DR
std::vector does a lot of copying and destroying of copies. In order to do this, objects contained by a vector must comply with one of the Rules of Three, Five, or Zero.
Because Player owns a pointer and uses the default copy functions the rule of 3/5/0 requirement is not observed. As a result, a copy's typeOfClass points to the same place as the source's typeOfClass. When either the source or the copy is destroyed, it deletes the typeOfClass that both source and copy are using, leaving the other pointing at invalid memory. The program is now broken and may or may not crash.
But if Player observes the Rules and has a move constructor and a move assignment operator like below
class Player{
public:
...
~Player();
Player(Player && src)
{
typeOfClass = src.typeOfClass;
src.typeOfClass = nullptr;
}
Player& operator=(Player && src)
{
typeOfClass = src.typeOfClass;
src.typeOfClass = nullptr;
}
private:
...
IClass* typeOfClass = new Warrior();
};
Then vector can move Players around without having two Players sharing the same data and Helper::Run can look like
void Helper::Run(){
playerList.emplace_back();
}
There will be no memory leaks and fewer pointers in play.
However... What you really want is to do as little special handling as possible. Every special case means more testing. If you can protect typeOfClass right at its source, then Player can be dumb as a post and adhere to the Rule of Zero.
In this case that means using a proxy object like a smart pointer to take ownership of typeOfClass and manage its lifespan for you. If all Warriors can have the same Warrior instance, that means std:shared_ptr. If Warriors need separate Warrior instances to handle bookkeeping for the containing Player instance you want a unique_ptr.
