why is deleting this object causing problems?

Question

Instantiation:

weapons.push_back(new Pistol());
weapons.push_back(new Rifle());
weapons.push_back(new Shotgun());

destructor, when the first delete happens, the code breaks. This happens when I close the program.

Brain::~Brain()
{
    for (unsigned int i = 0; i < weapons.size(); i++)
    {
        delete weapons[i]; // this is where the code breaks
    }
}

I get a warning:

Unhandled exception at 0x0096371f in D3D10DEMO.exe: 0xC0000005: Access violation reading location   0x000002ce.

weapons is this:

weapons(vector<Gun*>())

Edit - I have deleted much of the code from this question but I have also cut down my program so as to reproduce the problem in a much smaller solution here:

http://dl.dropbox.com/u/13519335/D3D10DEMO_0.25.MinRep.zip

The critics in the past was not that you didnt provide everything you have but that you were not presenting the problem in a concise and selfcontained way. Still not the case... — PlasmaHH, Nov 24 '11 at 12:39
A 23,813,003 byte zip? Files we need: `h`, `cpp`, `sln`, `vproj`, `vcxproj.*`, and `suo`. Files we do not need: `ipch`, `lastbuildstate`, `manifest`, `obj`, `pdb`, `sdf`, `log`, `tlog`, `idb`. Removing those brings it down to 67,724 bytes. Much more managable. Also downloads 351 times faster. — Mooing Duck, Dec 05 '11 at 19:22

score 7 · Answer 1 · answered Nov 24 '11 at 11:54

7

You haven't defined virtual destructors for your weapon classes. http://www.parashift.com/c++-faq-lite/virtual-functions.html#faq-20.7

answered Nov 24 '11 at 11:54

FeatureShock

118
5

Do I need to call the base destructor from the implementation of the derived destructor? – Dollarslice Nov 24 '11 at 13:15
Don't call the base destructor. http://stackoverflow.com/questions/677620/do-i-need-to-explicitly-call-the-base-virtual-destructor – FeatureShock Nov 24 '11 at 13:21
Gun-class is inherited from GameObject. Does it have virtual destructor? – FeatureShock Nov 24 '11 at 15:48
it does now, but the problem still occurs. I'll update the output from the call stack in my question – Dollarslice Nov 24 '11 at 15:58
About that Entity-class. Does it have virtual members? And virtual destructor? – FeatureShock Nov 24 '11 at 16:13
It has three members which all inherit from gameobject, all have implemented destructors. Entity doesn't have any virtual member though. – Dollarslice Nov 24 '11 at 16:40
So your call stack changed. Which line throws the exception this time? – FeatureShock Nov 25 '11 at 07:47
the same line; delete weapons[i] – Dollarslice Nov 25 '11 at 10:33
It hard to say what is wrong. By looking that stack trace, maybe there is a problem while deleting your agents vector? – FeatureShock Nov 25 '11 at 11:01
am I right in thinking that the derived class destructor, unlike most methods that override virtuals, does not have to have the same name as the base class destructor? – Dollarslice Nov 25 '11 at 12:53

parapura rajkumar · Accepted Answer · 2011-12-06T13:51:52.460

4

You problem is the definition of

class Brain : public Entity
{
private:
    std::vector<Gun*> weapons;

and the ownership of Gun* by Brain object.

Brain::~Brain()
{
    for (unsigned int i = 0; i < weapons.size(); i++)
    {
        delete weapons[i];
    }
}

If a Brain is copy constructed the delete will be called multiple times deleting the same Gun from different weapons vector. And a Brain temporary is created when you add your Agents (Agent being a derived class of Brain) like so in main function.

int main()
{
    Level* level;
std::vector<Agent> agents;

level = new Level(agents);

for (int i = 0; i < 1; i++)
    {
        //TODO - health is a pointless parameter here
        agents.push_back(Agent(100, *level, agents, level->Pickups(), D3DXCOLOR(1.0F, 0.4f, 0.4f, 1.0f)));
    }

delete level;

}

If you implement a copy constructor for Brain that clones the Gun* vector you should be ok. Alternative you should use shared_ptr<Gun> in your vector so that you don't have to delete them at all.

To summarize your problem boils down to

class Foo{};

class Bar
{
public:
    Bar()
    {
        mFooVec.push_back( new Foo() );
        mFooVec.push_back( new Foo() );
    }

    ~Bar()
    {
        for( unsigned int i = 0;i < mFooVec.size(); ++i )
        {
            delete mFooVec[i];
        }
    }

    std::vector<Foo*> mFooVec;
};

int main()
{
    Bar x;
    Bar y = x;

    return 0;
}

Here both Bar x and y have the same two Foo* in their mFooVec

edited Dec 06 '11 at 13:51

answered Dec 05 '11 at 18:59

parapura rajkumar

24,045
1
55
85

so the pointers are pointing at the same objects in all the classes. But there are also MORE pointers right? They just all point at the same thing, but there are actually different pointers in the different classes, yes? – Dollarslice Dec 06 '11 at 10:52
what is preferable? The copy constructor or the shared pointer? – Dollarslice Dec 06 '11 at 10:54
@SirYakalot: With or without copy constructor, use `std::vector> weapons;`, and do not delete `Gun` pointers. – ali_bahoo Dec 06 '11 at 11:28
@sad_man could you point me to some information on shared pointers and why they don't need to be deleted? also, sorry you're sad. – Dollarslice Dec 06 '11 at 12:08
1

@SirYakalot: It is a smart pointer that automatically deletes the pointer it owns. Read [this](http://stackoverflow.com/q/106508/425817) – ali_bahoo Dec 06 '11 at 12:27
OK the only things I still don't understand are - 1. you're saying I can create my own custom copy constructor to PROPERLY duplicate the pointers and the objects they're pointing to to get around this problem? 2. am I right in thinking that as it stands now, there are multiple sets of pointers but they all point to the same objects? 3. does this mean there are other objects in other class instances that have been created in a copy but that nothing is pointing to? 4. would I be right in thinking that a shared pointer would point all the classes at ONE set of objects? – Dollarslice Dec 06 '11 at 12:47
@SirYakalot As a thumb rule if a class has a pointer member , a array of pointers or a std container of pointer that it owns and needs to destroy when it gets destroyed it needs a copy constructor. If your design of class is such that it never will be copied... I typically provide a `declaration not definition of copy constructor and assignment operator` so that when such a class is copied I get a link error that it is not being used as designed. – parapura rajkumar Dec 06 '11 at 13:46

why is deleting this object causing problems?

2 Answers2

Linked