Serious bug...

112

Feb 24, 2005 at 10:48 AM

I have the following code:


template <class T>
class TransAnimationSource: public BaseClass {
	public:
		void SetImagesAmount (DWORD ImagesAmount);
		DWORD GetImagesAmount ();
		Real GetFPS();
		void KeepEnlargeImagesAmount (DWORD ImagesAmount);
		void ReadAdvancedRepresentationReduceFormat (ifstream & File);
		void SetFPS (Real FPS);
		void WriteAdvancedRepresentationReduceFormat (ofstream & File);
		void WriteImageAdvancedRepresentationReduceFormat (ofstream & File);

		TransImageSource & operator[] (DWORD Index);

		TransAnimationSource ();
		TransAnimationSource (Real FPS);
		~TransAnimationSource();
	private:
		Real FPS;
		StreamList <TransImageSource> ImageList;
};

StreamList<T>::~StreamList ()
{
	delete [] this->pObjects; // <=Runtime error
	this->ObjectsAmount = 0;
	this->pObjects = NULL;
}

	this->TransAnimation[0].~TransImageSource();
	this->TransAnimation.~TransAnimationSource();

I get this runtime error. If I omit this->TransAnimation[0].~TransImageSource(); then I dont get any error. However, If I put another this->TransAnimation[0].~TransImageSource(); then I dont get error on the second one (only the same error as the original code) which means its not a problem with the this->TransAnimation[0].~TransImageSource(); dstr. What can be causing this error?

FOUNDER

409

Feb 24, 2005 at 11:01 AM

One problem could be is that when you allocated new memory for pObjects, you did not use 'new []' but rather just 'new'. Using the different 'delete' operations on the seperate methods of memory allocation is known to cause nasty crashes.

- Drew

jflanglois

Feb 24, 2005 at 11:05 AM

There isn't enough information there for me to help you (What runtime error is it, exactly? What does the TransImageSource destructor do? Why are you calling the destructor manually, and where?). Try stepping through your code in a debugger and see what is causing the runtime error (I'm betting on an invalid pointer).

Sorry I couldn't help more.

Regards,
jflanlgois

Author

112

Feb 24, 2005 at 11:21 AM

I have create a Test method and replaced the dstr with Test:

class TransImageSource: public BaseClass {	public:		void CenterPosition (signed long & PosX, signed long & PosY);		void ClearPointers();		void CreateMaskImage (MaskImage & MskImg);		void CreateARGBImage (DWORD & Width, DWORD & Height, DWORD & BlankStartX, DWORD & BlankStartY, DWORD & OriginialWidth, DWORD & OriginalHeight, signed long & StartX, signed long & StartY, BYTE * pPixels);		void Crop ();		void GetDimentions (DWORD & SizeX, DWORD & SizeY);		void GetRealSize (DWORD & SizeX, DWORD & SizeY);		void GetStart (signed long & StartX, signed long & StartY);		void InitializeTransImage (DWORD Width, DWORD Height);		void InitializeTransImage (RGBImage & AlphaImage, RGBImage & ColorImage);		void ReadAdvancedRepresentationReduceFormat (ifstream & File);		void RestoreTransColors (DWORD Color);		void WriteAdvancedRepresentationReduceFormat (ofstream & File);		void Test();		TransImageSource & operator= (const TransImageSource & Source);		TransImageSource ();		TransImageSource (DWORD Width, DWORD Height);		~TransImageSource();	private:		DWORD OriginalWidth, OriginalHeight, BlankStartX, BlankStartY;		signed long StartX, StartY;		RGBImage Colors;		AlphaImage Alpha;		MaskImage FullMask, TransMask;		FastSquareRoot SRoot;		void CopyCrop (TransImageSource & Source);};TransImageSource::~TransImageSource(){}voidTransImageSource::Test(){	this->Colors.~RGBImage();	this->Alpha.~AlphaImage();	this->FullMask.~MaskImage();	this->TransMask.~MaskImage();	this->SRoot.~FastSquareRoot();}void TransAnimationSource::SetImagesAmount (DWORD ImagesAmount){	this->ImageList.Allocate (ImagesAmount);}template <class T>voidStreamList<T>::Allocate (DWORD ObjectsAmount){	delete[] this->pObjects;	this->ObjectsAmount = ObjectsAmount;	this->pObjects = new T [ObjectsAmount];}	ImagesAmount=1;	this->TransAnimation.SetImagesAmount (ImagesAmount);//	this->TransAnimation[0].~TransImageSource();	this->TransAnimation[0].Test();	this->TransAnimation.~TransAnimationSource();

When I use Test instead of ~TransImageSource() there is no error!
Altough they are suppose to be identical!

Author

112

Feb 24, 2005 at 11:37 AM

I am not sure I understand whats going on.
When I debug and press go I get the following message:
"User breakpoint called from code at 0x77f767cd"
I press OK, the debugger has stop on delete [] this->pObjects; like I showed earlier.
If I press F10 then the debugger goes to the next code line, but deeper in the call stack on some assembly code.
Then I Press Ctrl+F10 on the line of code after this->TransAnimation.~TransAnimationSource(); in the respective call stack depth and I get the following error dialog box:
Debug Assertion Failed!

Program: E:\PROJECT\Engine2\V0\Debug\V0.exe
File: dbgheap.c
Line: 1044

Expression: _CrtIsValidHeapPointer(pUserData)

I press retry and the pointer of the debugger is on the same line of code it was before (in the appropriate depth of the stack), which is:
delete [] this->pObjects;

FOUNDER

409

Feb 24, 2005 at 11:55 AM

I think what is happening is that you are calling:

delete [] this->pObjects;

More than once. That would explain the crash. Try this:

delete [] this->pObjects;this->pObjects = 0;

And see if you get any more crashes.

526

Feb 24, 2005 at 12:01 PM

You shouldn't directly invoke destructors. Either have automatic storage go out of scope, or call delete on dynamic storage.

238

Feb 24, 2005 at 12:10 PM

If you manually call the destructor, the data will be called delete [] on. When the destructor then gets called automatically at end of scope or upon a delete of the object, it calls delete [] on the data again, which makes it crash.

That is, your line only crashes the second time the destructor is called.

And Oluseyi is right, unless you are using placement new, don't call destructors manually ever.

Author

112

Feb 24, 2005 at 12:35 PM

Quote:
Original post by ToohrVyk
If you manually call the destructor, the data will be called delete [] on. When the destructor then gets called automatically at end of scope or upon a delete of the object, it calls delete [] on the data again, which makes it crash.

That is, your line only crashes the second time the destructor is called.

And Oluseyi is right, unless you are using placement new, don't call destructors manually ever.

Which manual dstr call do you refer to?
When I call this->TransAnimation[0].~TransImageSource();
it shouldnt invoke any delete[] on the dynamic data it is stored at.
When I call this->TransAnimation.~TransAnimationSource() the delete[] is called for the first time and there is where the error appears.
Additionaly I set the data which I call delete[] upon to NULL, so there shouldnt be aproblem calling the dstr the second time, as you can see in:

template <class T>StreamList<T>::~StreamList (){	delete [] this->pObjects;	this->ObjectsAmount = 0;	this->pObjects = NULL; // <= Nullifing}

Author

112

Feb 24, 2005 at 1:13 PM

Is the destructor some sort of mystic method that no one knows exactly how it works and therfore advise to avoid from invoking it?!
I could just add some sort of unavail method that will do the job that the destructor supposed to do without causing cahos in the gentle fabric of C++.
Because without imitating the job of the destructor you cant reuse members which are class, and have to relaese the object and the memory which is upon and allocate new memory to a new object instead of reusing the memory of the object which you had already allocated.
If thats the case, then C++ is not such a smart language.

382

Feb 24, 2005 at 1:16 PM

Quote:
Original post by The C modest god
Is the destructor some sort of mystic method that no one knows exactly how it works and therfore advise to avoid from invoking it?!

No, the compiler calls it for you automaticly when the variable goes out of scope or is delete or delete[]ed.

183

Feb 24, 2005 at 1:21 PM

As said several times already, the compiler invokes the destructor of an object for you when it gets destroyed. With a few rare exceptions, you should never call it yourself.

Author

112

Feb 24, 2005 at 1:22 PM

Quote:
Original post by Evil Steve
Quote:
Original post by The C modest god
Is the destructor some sort of mystic method that no one knows exactly how it works and therfore advise to avoid from invoking it?!
No, the compiler calls it for you automaticly when the variable goes out of scope or is delete or delete[]ed.

but you dont know exactly what the destructor does, because you wont risk calling it explictly.
So a destructor does more then just calling the destructors of all the objects sub class members. What he does more? maybe no one exactly knows.

outRider

110

outRider

Feb 24, 2005 at 1:22 PM

Do not call it yourself. What part of that idea is giving you trouble?

And,

// this->TransAnimation[0].~TransImageSource();
this->TransAnimation[0].Test();
this->TransAnimation.~TransAnimationSource();

What's the meaning of that? Is TransAnimation an array/pointer or an actual object? You can't treat it as both.

My guess is that you're calling the destructor more than once on an auto object.

382

Feb 24, 2005 at 1:25 PM

Quote:
Original post by The C modest god
I could just add some sort of unavail method that will do the job that the destructor supposed to do without causing cahos in the gentle fabric of C++.
Because without imitating the job of the destructor you cant reuse members which are class, and have to relaese the object and the memory which is upon and allocate new memory to a new object instead of reusing the memory of the object which you had already allocated.
If thats the case, then C++ is not such a smart language.

If you want to reuse the memory, then you're writing a memory manager, and it'll be valid to use placement new (which calls the constructor) and to call the destructor yourself. But you'd also never use new or delete, except to allocate x bytes of memory (or malloc()).
This is what most people have functions like Init() and Shutdown() for. If you want to reuse the class, you can either clean it up and start again, or you can delete it an re-create it.

382

Feb 24, 2005 at 1:27 PM

Quote:
Original post by The C modest god
So a destructor does more then just calling the destructors of all the objects sub class members. What he does more? maybe no one exactly knows.

Calling the destructor for a class calls the destructor for all of the member variables in the class, and calls the base classes destructor. That's all.
If you call the destructor for a class containing a std::string, then the STL will just free the memory associated with the string. It won't set the pointers to NULL, to save some time.
Then you delete the object, and the CRT calls the destructor for your class again. That causes your std::string member to have it's destructor called again, and it'll attempt to free memory that has already been freed.

EDIT:

Quote:
Original post by The C modest god
but you dont know exactly what the destructor does, because you wont risk calling it explictly.

See above for what it does. I never call it explicitly because I don't need to, and doing so will cause errors.
In the same way I never put this at the start of my functions: int* p = new int; delete p; *p = 42;

238

Feb 24, 2005 at 1:37 PM

Quote:
Original post by The C modest god
Because without imitating the job of the destructor you cant reuse members which are class, and have to relaese the object and the memory which is upon and allocate new memory to a new object instead of reusing the memory of the object which you had already allocated.
If thats the case, then C++ is not such a smart language.

This is what operator= is used for.

Author

112

Feb 24, 2005 at 1:41 PM

Quote:
Original post by Evil Steve
Quote:
Original post by The C modest god
So a destructor does more then just calling the destructors of all the objects sub class members. What he does more? maybe no one exactly knows.
Calling the destructor for a class calls the destructor for all of the member variables in the class, and calls the base classes destructor. That's all.
If you call the destructor for a class containing a std::string, then the STL will just free the memory associated with the string. It won't set the pointers to NULL, to save some time.
Then you delete the object, and the CRT calls the destructor for your class again. That causes your std::string member to have it's destructor called again, and it'll attempt to free memory that has already been freed.

EDIT:
Quote:
Original post by The C modest god
but you dont know exactly what the destructor does, because you wont risk calling it explictly.
See above for what it does. I never call it explicitly because I don't need to, and doing so will cause errors.
In the same way I never put this at the start of my functions: int* p = new int; delete p; *p = 42;

Doesnt calling the two following functions should give the same results?

TransImageSource::~TransImageSource(){}voidTransImageSource::Test(){	this->Colors.~RGBImage();	this->Alpha.~AlphaImage();	this->FullMask.~MaskImage();	this->TransMask.~MaskImage();	this->SRoot.~FastSquareRoot();}

Then why one cause afterwards an error and the second doesnt?

526

Feb 24, 2005 at 1:45 PM

Quote:
Original post by The C modest god
Is the destructor some sort of mystic method that no one knows exactly how it works and therfore advise to avoid from invoking it?!

You're suffering from bad design and worse knowledge. Ranting about the destructor is beside the point. A destruct destroys an object; it doesn't "clear it out" for "reuse."

What you really want is state management of your objects, where you can mark the object "dead" or "done" and it can be repurposed. That's nothing new. In fact, that's trivial, and it's Good Design™. The alternative is to use placement new, along with pool memory allocation. That's not quite as trivial, but it is also Good Design™. Note, however, that recreating the entire object in-place rather than replacing its data and altering its state is best done when you need the actual object to be recreated. From the source you've shown, that is not the case.

In short order, write better code. Happy hacking.

183