CODEWITHSUNDEEP
c++memory-leaks
Diogo Lacerda AltTabs
Diogo Lacerda AltTabs

Reputation: 13

C++ Memory Leak on function argument

So i will get straight to the point: I have a function that is adding some stuff (which happens to be a class) to another class:

tower->set_weapon(new NoWeapon());

The set_weapon() is a simple method in the tower class, which associated a inner variable with this new NoWeapon class it receives.

virtual void set_weapon(Weapon* weapon){ weapon_ = weapon; };

weapon_ is a pointer to the Weapon class, and NoWeapon is hierarchically the son of weapon. When everything is said and done, the tower itself cleans everything up:

weapon_->Clean();
delete weapon_;

However, im still getting a memory leak. Im using vld on VisualStudio 2013, and it detects a memory leak on the first line i mentioned

tower->set_weapon(new NoWeapon());

Any idea why this is happening and how could i work around it?

Thank you in advance!

EDIT: Thanks to YSC for the solution. I was in fact getting a leak over not initializing the original weapon pointer, but your ideas got me back on track.

As for the unique_ptr ideas, i played with it before, but it wasn't the solution i was looking for. Thanks for your help anyway.

Upvotes: 1

Views: 469

Answers (2)

YSC
YSC

Reputation: 40060

We lack context information, but basically if Tower::set_weapon() is called twice, Tower::weapon_ get overwritten and memory leaks. Suggestion:

Tower::Tower()
    : weapon_(nullptr)
{}

void Tower::set_weapon(Weapon* weapon)
{
    if (weapon_ != nullptr)
    {
        weapon_->Clean(); // should be called by Weapon::~Weapon()
    }
    delete weapon_;
    weapon_ = weapon;
};

You could also use an std::unique_ptr:

class Tower
{
    std::unique_ptr<Weapon> weapon_;

public:
    Tower::Tower() {}

    void set_weapon(std::unique_ptr<Weapon> weapon)
    {
        // assume Weapon::~Weapon() is fixed
        weapon_ = std::move(weapon);
    };
};

Upvotes: 3

Richard Hodges
Richard Hodges

Reputation: 69854

note: YSC's answer is correct. A weapon should not need a Clean method as it can handle resource deallocation in a destructor.

However, if there really is some reason for the call to Clean, you can encapsulate all this in std::unique_ptr with a custom deleter.

like so:

struct weapon_cleaner
{
  void operator()(Weapon* pw) const nothrow {
    if (pw) {
      try {
        pw->Clean();
      }
      catch(...) {
      }
    }
    delete pw;
  }
};

using weapon_pointer = std::unique_ptr<Weapon, weapon_cleaner>;

class Tower
{
    weapon_pointer weapon_;

public:
    Tower::Tower() {}

    void set_weapon(weapon_pointer weapon)
    {
        weapon_ = std::move(weapon);
    };
};

called like so:

weapon_pointer make_machine_gun() {
  return weapon_pointer(new MachineGun(), 
                        weapon_cleaner());
}

mytower.set_weapon(make_machine_gun());

Upvotes: 1

Related Questions