0

So i was doing some assignment and suddenly facing this "invalid pointer" error when I try to "delete" the earlier assigned weaponBehaviour in the "setweapon" function in class Character . Could someone give some pointers as to what might be the problem ?


#include <iostream>
using namespace std;

class WeaponBehaviour{
public:
    virtual void useWeapon() = 0;
};

class SwordBehaviour : public WeaponBehaviour {
public:
    void useWeapon()
    {
        cout << "slash slash"<< endl;
    }
};
class BowAndArrowBehavior : public WeaponBehaviour {
public:
    void useWeapon()
    {
        cout << "suss suss"<< endl;
    }
};

class KnifeBehavior : public WeaponBehaviour {
public:
    void useWeapon()
    {
        cout << "chak chakk"<< endl;
    }
};
class AxeBehavior : public WeaponBehaviour {
public:
    void useWeapon()
    {
        cout << "chop chop"<< endl;
    }
};



class Character{
protected:
    WeaponBehaviour* weaponBehaviour;
public:
    Character(){

    }
    ~Character(){
        if(weaponBehaviour != NULL)
        {
            delete weaponBehaviour;
        }
    }
    void setWeapon(WeaponBehaviour* newWeapon)
    {
        if(weaponBehaviour!= NULL )
            delete weaponBehaviour;

        weaponBehaviour = newWeapon;
    }
    void fight()
    {
        weaponBehaviour->useWeapon();
    }
};

class King : public Character
{
public:
    King(){
        setWeapon(new SwordBehaviour);
    }
};

class Queen : public Character
{
public:
    Queen(){
        setWeapon(new KnifeBehavior);
    }
};

int main() {
    King king ;
    king.fight();
    Queen queen ;
    queen.fight();
    queen.setWeapon(new AxeBehavior);
    queen.fight();
    return 0;
}

the error i am getting is

* Error in `Cpp-design-patters/Debug/strategy-pattern': munmap_chunk(): invalid pointer: 0x0000000000400f50 * ======= Backtrace: ========= /lib/x86_64-linux-gnu/libc.so.6(+0x777e5)[0x7ff746d757e5] /lib/x86_64-linux-gnu/libc.so.6(cfree+0x1a8)[0x7ff746d82698]

hungryspider
  • 300
  • 2
  • 13
  • 3
    You should try using a debugger. It's an invaluable tool for this kind of issue. Also compile with warnings. You have no virtual destructor in `WeaponBehaviour`, so the deletes in your code are UB. – Paul Rooney Oct 17 '19 at 06:14
  • 1
    If I read that backtrace correctly (its formatting is a bit messed up), then this is far away from your code. Your code (you should take it to codereview.stackexchange.com, btw, there's lots to improve) doesn't look like it could cause that to me. The only obvious mistake is the lack of virtual destructors. – Ulrich Eckhardt Oct 17 '19 at 06:14
  • @PaulRooney thanks for the tip ,the issue was because of not using virtual destructor adding it solved the issue . – hungryspider Oct 17 '19 at 06:23
  • ok cool. I added it but still got a segfault. You should pass `-Wall -Wextra -pedantic` flags to GCC and also `-g` to include debug symbols. With this GCC warns about the dtor. – Paul Rooney Oct 17 '19 at 06:24
  • 3
    you never initialize `weaponBehaviour` to `NULL`, it has an indeterminate value that is likely *not* NULL, so you'll try to `delete` something that you never `new`'d – kmdreko Oct 17 '19 at 06:30
  • In order to debug things like that it is a good idea to use valgrind (http://valgrind.org/). That has a good chance to tell you exactly what went wrong. – Daniel Junglas Oct 17 '19 at 06:41

1 Answers1

0

You need to initialise weaponBehaviour to nullptr otherwise weaponBehaviour will have an undefined (possibly non-null) value and

if(weaponBehaviour!= NULL)
  delete weaponBehaviour;

Will pass the if statement and delete an invalid pointer.

You compiler should also warn you (if you have turned on warnings) that you have a virtual class with no virtual destructor. This leads to undefined behaviour when deleting an object via a WeaponBehaviour pointer. The correct declaration of WeaponBehaviour is:

class WeaponBehaviour{
public:
    virtual WeaponBehaviour() {}
    virtual void useWeapon() = 0;
};

It isn't causing a problem in your code (yet) but you should be aware of the rule of three/five.

All of the above would be solved by using std::unique_ptr and would make your code simpler and safer:

#include <iostream>
#include <memory>

class WeaponBehaviour{
public:
    virtual ~WeaponBehaviour(){}
    virtual void useWeapon() = 0;
};

class SwordBehaviour : public WeaponBehaviour {
public:
    void useWeapon()
    {
        std::cout << "slash slash\n";
    }
};
class BowAndArrowBehavior : public WeaponBehaviour {
public:
    void useWeapon()
    {
        std::cout << "suss suss\n";
    }
};

class KnifeBehavior : public WeaponBehaviour {
public:
    void useWeapon()
    {
        std::cout << "chak chakk\n";
    }
};
class AxeBehavior : public WeaponBehaviour {
public:
    void useWeapon()
    {
        std::cout << "chop chop\n";
    }
};



class Character{
protected:
    std::unique_ptr<WeaponBehaviour> weaponBehaviour;
public:
    void setWeapon(std::unique_ptr<WeaponBehaviour>&& newWeapon)
    {
        weaponBehaviour = std::move(newWeapon);
    }
    void fight()
    {
        weaponBehaviour->useWeapon();
    }
};

class King : public Character
{
public:
    King(){
        setWeapon(std::make_unique<SwordBehaviour>());
    }
};

class Queen : public Character
{
public:
    Queen(){
        setWeapon(std::make_unique<KnifeBehavior>());
    }
};

int main() {
    King king ;
    king.fight();
    Queen queen ;
    queen.fight();
    queen.setWeapon(std::make_unique<AxeBehavior>());
    queen.fight();
    return 0;
}
Alan Birtles
  • 32,622
  • 4
  • 31
  • 60