Skip to main content
gamedev.net
🔒 Locked

HelP! (Access Violation ) delete pointer

Started by fantasyme May 28, 2010 at 7:21 AM 12 replies 3.3k views
Original Post
fantasyme
fantasyme
I have 2 classes , one is base class and the another one is derive class. 


class CCharacter
{
protected:
	
	CMakePolygon*		p_tex;						
	CLoadTextureClass*	t_tex;						
	int					status;							

public:
	CCharacter(void);
	virtual ~CCharacter(void){CleanUp();}
	virtual void Init();
	virtual void Draw();
	virtual void Main();
	virtual void CleanUp(){delete p_tex; delete t_tex;}
	void SetStatus(int n){status = n;}										
};



class CLoading: public CCharacter
{
private:
	int					FPS;												
	int					round;												
public:
	CLoading(void);
	~CLoading(void){CleanUp();}

	void Init();
	void Main();
	void Animation();
};
the program works but when I close the program I got Access Violation: 0xC0000005: on CCharacter::CleanUp function What did I do wrong?
Psycrosis
Psycrosis
CleanUp is being called twice.... once in the inherited destructor and in the base destructor... So the pointers are being deleted twice, hence the access violation... You can take it out of one of the destructor's or make the pointers null after you delete them... that way it won't matter if you call delete on them again...
fantasyme
fantasyme
Quote:
Original post by Psycrosis
CleanUp is being called twice.... once in the inherited destructor and in the base destructor... So the pointers are being deleted twice, hence the access violation... You can take it out of one of the destructor's or make the pointers null after you delete them... that way it won't matter if you call delete on them again...


Thanks for you advice.
but I don't understand what you mean?
do you mean? I should delete one destructor like no destrutor in derived class?
I delete destrutor in derived class(CLoading class) but it still got the problem
Evil Steve
Evil Steve
Quote:
Original post by fantasyme
Quote:
Original post by Psycrosis
CleanUp is being called twice.... once in the inherited destructor and in the base destructor... So the pointers are being deleted twice, hence the access violation... You can take it out of one of the destructor's or make the pointers null after you delete them... that way it won't matter if you call delete on them again...


Thanks for you advice.
but I don't understand what you mean?
do you mean? I should delete one destructor like no destrutor in derived class?
I delete destrutor in derived class(CLoading class) but it still got the problem

Instead of:
virtual void CleanUp(){delete p_tex; delete t_tex;}
You should do:
virtual void CleanUp(){delete p_tex; p_tex=NULL; delete t_tex; t_tex=NULL;}

Otherwise you can end up calling delete p_tex when it's already been deleted - calling delete on a NULL pointer is perfectly valid, and will do nothing.
SiCrane
SiCrane
Better yet, replace the raw pointers with smart pointers and don't worry about explicit clean up.
Psycrosis
Psycrosis
When you have a virtual destructor in a base class it is called when the derived class is deleted or goes out of scope. When that happens the function CleanUp() is being called twice, hence pointers being deleted twice. What I am saying is the call to the CleanUp function only needs to be done in either the base or inherited destructor but not both. You should null out your pointers too as it is good coding practice. What you may have intended is a seperate cleanup for each destructor, for that you MUST override the CleanUp function in the inherited class or the base CleanUp will be called twice.

Here's an example..

class CCharacter
{
protected:

CMakePolygon* p_tex;
CLoadTextureClass* t_tex;
int status;

public:
CCharacter(void);
virtual ~CCharacter(void){CleanUp();}
virtual void Init();
virtual void Draw();
virtual void Main();
virtual void CleanUp(){delete p_tex; delete t_tex;}
void SetStatus(int n){status = n;}
};



class CLoading: public CCharacter
{
private:
int FPS;
int round;
public:
CLoading(void);
~CLoading(void){CleanUp();}

void Init();
void Main();
void Animation();
// Add this!
virtual void CleanUp(){}
};

[Edited by - Psycrosis on May 28, 2010 8:05:10 AM]
delta user
delta user
Psycrosis: True

fantasyme: You also need to be very careful when calling virtual functions inside a destructor as this might not do what you expect it to do.
Someone who uses a, euhm..., delta!?
fantasyme
fantasyme
Quote:
Original post by Evil Steve
Quote:
Original post by fantasyme
Quote:
Original post by Psycrosis
CleanUp is being called twice.... once in the inherited destructor and in the base destructor... So the pointers are being deleted twice, hence the access violation... You can take it out of one of the destructor's or make the pointers null after you delete them... that way it won't matter if you call delete on them again...


Thanks for you advice.
but I don't understand what you mean?
do you mean? I should delete one destructor like no destrutor in derived class?
I delete destrutor in derived class(CLoading class) but it still got the problem

Instead of:
virtual void CleanUp(){delete p_tex; delete t_tex;}
You should do:
virtual void CleanUp(){delete p_tex; p_tex=NULL; delete t_tex; t_tex=NULL;}

Otherwise you can end up calling delete p_tex when it's already been deleted - calling delete on a NULL pointer is perfectly valid, and will do nothing.


thanks for your help,
but I still got error.
Unhandled exception at 0x0018355e in RenewOracle.exe: 0xC0000005: Access violation reading location 0xcccccccc.

I did as you advise.
the problem is still delete p_tex;
Evil Steve
Evil Steve
Quote:
Original post by fantasyme
but I still got error.
Unhandled exception at 0x0018355e in RenewOracle.exe: 0xC0000005: Access violation reading location 0xcccccccc.
That means that the pointer was never initialised. In your constructor, you should be initialising all your pointers to NULL.

Or, as SiCrane said - use a smart pointer class so you don't have to worry about this sort of thing.
Psycrosis
Psycrosis
Or switch to a garbage collected language like (insert favorite .Net language here),Java or D and not worry about it lol.... Thats one thing I love about programming in C# is that Visual Studio warns you about not initializing something while you are coding...
fantasyme
fantasyme
Quote:
Original post by Evil Steve
Quote:
Original post by fantasyme
but I still got error.
Unhandled exception at 0x0018355e in RenewOracle.exe: 0xC0000005: Access violation reading location 0xcccccccc.
That means that the pointer was never initialised. In your constructor, you should be initialising all your pointers to NULL.

Or, as SiCrane said - use a smart pointer class so you don't have to worry about this sort of thing.


ok, I put p_tex and t_tex = NULL in constructor
and I got error :: Debug Assertion Failed
Expression BLOCK_TYPE_IS_VALID(pHead->nBlockUse)

and what is smart pointer class that you mention?
shobogenzo
shobogenzo
Fantasyme,

The simplest solution to this problem is simply to take out CLoading's call to Cleanup():

class CLoading: public CCharacter
{
private:
...

public:
CLoading(void);
virtual ~CLoading(void){}
...
};

You don't need to call Cleanup in CLoading since the base CCharacter destructor calls it.

But also, ideally you should address the following:
1) Why is Cleanup public?
2) And why don't you set the pointers to Null after delete? Or...
3) Why don't you reinitialize another set of objects in Cleanup?
4) Instead of Cleanup, you could have a public method called CleanupAndInit that not only deletes, but reinitializes memory for p_tex and t_tex. Then, in your CCharacter destructor, simply delte p_tex and t_tex directly. Your new CCharacter class would look something like this:

class CCharacter
{
protected:
CMakePolygon* p_tex;
CLoadTextureClass* t_tex;
...
public:
CCharacter(void) { p_tex = NULL; t_tex = NULL; }
virtual ~CCharacter(void){delete p_tex; delete t_tex;}
virtual void CleanUpAndInit() {
if(p_tex != NULL) delete p_tex;
if(t_tex != NULL) delete t_tex;
p_tex = new CMakePolygon();
t_tex = new CLoadTextrueClass();
...
}
...
...
};
iMalc
iMalc
There are several things you should learn, given what you've posted in the following snippet of code:
Quote:
Original post by shobogenzo

CCharacter(void) { p_tex = NULL; t_tex = NULL; }
virtual ~CCharacter(void){delete p_tex; delete t_tex;}
virtual void CleanUpAndInit() {
if(p_tex != NULL) delete p_tex;
if(t_tex != NULL) delete t_tex;
p_tex = new CMakePolygon();
t_tex = new CLoadTextrueClass();
...
}<!--QUOTE-->

(void) is an old C throwback. In C++, just use ()
Constructor initialisation lists are the perferred way to initialised member variables:
CCharacter() : p_tex(NULL), t_tex(NULL) {}
Don't check for a pointer being NULL before calling delete. It is completely normal and safe to do "delete NULL;".
CleanUpAndInit is not exception safe. The better way to do the above is to use the copy-and-swap idiom.
Use [code]code goes here[/code] or [source]code goes here[/source] tags when posting code. This ensures that indentation is retained.

fantasyme: When you have a class where the following compiles but causes, or would cause, a problem at run-time, then in my book are duty-bound to either fix it, or prevent copying:
{    MyClass a;    MyClass b = a;    a = b;}
The later is done as follows... Note the private section:
class MyClass {    public:        OtherClass *somePointer;        // constructor, destructor, other methods, etc    private:        MyClass(const MyClass &);        MyClass& operator=(const MyClass &);}
fantasyme
fantasyme
Quote:
Original post by shobogenzo
Fantasyme,

The simplest solution to this problem is simply to take out CLoading's call to Cleanup():

class CLoading: public CCharacter
{
private:
...

public:
CLoading(void);
virtual ~CLoading(void){}
...
};

You don't need to call Cleanup in CLoading since the base CCharacter destructor calls it.

But also, ideally you should address the following:
1) Why is Cleanup public?
2) And why don't you set the pointers to Null after delete? Or...
3) Why don't you reinitialize another set of objects in Cleanup?
4) Instead of Cleanup, you could have a public method called CleanupAndInit that not only deletes, but reinitializes memory for p_tex and t_tex. Then, in your CCharacter destructor, simply delte p_tex and t_tex directly. Your new CCharacter class would look something like this:

class CCharacter
{
protected:
CMakePolygon* p_tex;
CLoadTextureClass* t_tex;
...
public:
CCharacter(void) { p_tex = NULL; t_tex = NULL; }
virtual ~CCharacter(void){delete p_tex; delete t_tex;}
virtual void CleanUpAndInit() {
if(p_tex != NULL) delete p_tex;
if(t_tex != NULL) delete t_tex;
p_tex = new CMakePolygon();
t_tex = new CLoadTextrueClass();
...
}
...
...
};


thank to you and everyone.
It is ok now.

It is good to know about this.

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.

Sign in to reply to this topic.