Sign in to follow this  

Weird problem with delete

This topic is 4613 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hello, I have a class with the following constructor:
CShipDrone::CShipDrone (void)
{
	CWeapon **weapons = new CWeapon*[1];
	weapons[0] = new CWpnLaser();
	delete weapons[0];
	delete weapons;
}
Doesn't do much, it's just for debugging the error I'm getting. When I create an instance of this class (CShipDrone) and then delete it, like so: CShip *ship = new CShipDrone(); delete ship; the game crashes on the 'delete ship' statement. The class CShipDrone is based on the CShip class, and the CWpnLaser class is based on the CWeapon class. It is interesting that this code works fine when I allocate an instance of the base class, CWeapon, in the constructor rather than the derived class CWpnLaser. Also, there is no crashing if I comment out either 'delete weapons[0]' or 'delete weapons' in the constructor. Why would this be crashing only when I use CWpnLaser? And why does it crash only when I delete the instance of the class that made the allocation? Thank You! Jon Woyame

Share this post


Link to post
Share on other sites
try this


CShip *ship = NULL;
ship = new CShipDrone();
delete ship;

if that doesnt crash then its not being properly allocated

Share this post


Link to post
Share on other sites
Sorry I read through quite fast, but from the code snippet it seems that youre allocating with new[] and deleting with the standard delete.

try replacing delete weapons; by delete[] weapons;

Hope this helps

Eric

Share this post


Link to post
Share on other sites
Still crashes.

I also forgot to mention that it doesn't have anything to do with the CShipDrone class, because it has the same problem when I put the code in another class's constructor and create an instance of it.

Jon

Share this post


Link to post
Share on other sites
the last line of your constructor should be

delete [] weapons;

since you are technically allocating an array (of one)
also you have not posted your destructor code.

If you are inheriting from CShip and CWeapon make sure your destructors there are virtual.

Share this post


Link to post
Share on other sites
Quote:
Original post by xEricx
try replacing delete weapons; by delete[] weapons;


I didn't notice that, but your right. However the game still crashes when I change it to delete[].

None of the classes involved have destructors.

Share this post


Link to post
Share on other sites
There are two potential problems here. One is a memory management issue, the other is a faulty destructor.

If it's CWpnLaser's destructor, then the following code should also crash:


CShipDrone::CShipDrone(void)
{
CWpnLaser foo;
}


If it's an allocation issue, start small and work your way up:


CShipDrone::CShipDrone(void)
{
CWeapon* weapon = new CWpnLaser();
delete weapon;
}


If the above code fails, but the first set works... well... sounds like an "operator new" thing. I haven't worked with custom memory managers much, so I really can't help you there.


How about this:

CShipDrone::CShipDrone(void)
{
std::vector<CWeapon*> weapons;
weapons.push_back( new CWpnLaser() );
delete weapons[0];
}


Don't mess with dynamic memory allocation when you don't have to. STL containers and boost::smart_ptr will virtually remove your need to ever call "delete".

http://www.boost.org/libs/smart_ptr/smart_ptr.htm

Share this post


Link to post
Share on other sites
First, it would be easier to change this to a :

std::vector<CWeapon* > m_pWeapons;

then, m_pWeapons.push_back(new CWpnLaser());

when destroying your ship, iterate through every items in m_pWeapons and delete them;

Also, when does it crash? In CShipDrone's ctor? in CShipDrone's dtor? or in another moment?

Can you also give us the error message you get when it crashes? Did you try to use a debugger?

Thanks

Share this post


Link to post
Share on other sites
Is CWpnLaser class an inherited class from CWeapon? Because... weapons[0] is a pointer to a CWeapon, yet in your new you say CWpnLaser.

I usually try to avoid double pointers when I can, there is usually a better way of doing things. Also I would add to your code a NULL check.

Share this post


Link to post
Share on other sites
This code made it crash:

CShipDrone::CShipDrone(void)
{
CWeapon* weapon = new CWpnLaser();
delete weapon;
}

However it didn't crash with this:

CShipDrone::CShipDrone(void)
{
CWpnLaser foo;
}

It doesn't crash with this either:

CShipDrone::CShipDrone(void)
{
CWeapon* weapon = new CWpnLaser();
}

This is a very strange problem. I've never seen anything like this.

Jon

Share this post


Link to post
Share on other sites
I think I'm close, after some debugging. Both the CWpnLaser class and the CShip class have a member variable of type CTimer. In the destructor of CTimer, the timer removes itself from a linked list of timers. In the code below,

CWeapon* weapon = new CWpnLaser();
delete weapon;

the call to 'delete weapon' destroys the CTimer member variable for the CWpnLaser, but it doesn't call its destructor. Thus, the timer is not removed from the linked list. When this code is run,

CShip *ship = new CShipDrone();
delete ship;

the destructor for the CShip class's CTimer is called, but the next pointer in the list is no longer valid (because the deconstructor was never called to remove that link). Hence the crash on the call to 'delete ship'.

I'm guessing the deconstructor is not getting called because the CTimer is a member of CWpnLaser and not its base class, CWeapon, and the delete statement is made on a CWeapon pointer. The fact that changing to pointer type from CWeapon to CWpnLaser doesn't cause a crash would support this.

Am I right about why the deconstructor is not getting called? What is the best way to fix this without changing the pointer type?

Thanks for sticking with me. Sometimes I hate computers.

Jon

Share this post


Link to post
Share on other sites
Seems to me like you should add destructors for your Weapons.

Have the base class remove the timer, and make destructors of child classes virtual.

If that's what was causing your problems, this should solve it.

Hope this helps

Eric

Share this post


Link to post
Share on other sites
Quote:
Original post by JonWoyame
Right, no deconstructors.


Quote:
Original post by JonWoyame
Am I right about why the deconstructor is not getting called? What is the best way to fix this without changing the pointer type?


Wright, well you do relize that invoking delete on a pointer to some base type where the base type does not declare the destructor virtual is undefined behaviour. Make the base type destructor virtual if your going to invoke delete on pointers to base types.

Share this post


Link to post
Share on other sites
Sign in to follow this