C++ Question Avoid Memory Leakages

[fred18]

Hi all,
I setup the payload rotation within MS2015, but looks like an MGROUP_ROTATE doesn't like to be redefined with the same name for each mesh and has to be static.

Therefore I had only the option to have a different MGROUP_ROTATE class for each mesh or using the following structure:

MGROUP_ROTATE *payloadrotatex=new MGROUP_ROTATE(payload[pns].msh_idh[nm],NULL,NULL,reference,_V(1,0,0),(float)2*PI);

Now, from what I learned during this years developing it's a responsability of the good programmer to avoid memory leakages, but if, at the end of the function, I call

delete payloadrotatex;

I get a CTD.

How can I do it safely?

Thanks in advance!

 

[meson800]

Not sure why that's CTDing, but why not just create the MGROUP on the stack?

In otherwords, just do

MGROUP_ROTATE payloadrotatex = MGROUP_ROTATE(payload[pns].msh_idh[nm],NULL,NULL,reference,_V(1,0,0),(float)2*PI);

Then if you're passing it to a function that needs a pointer to it, just do it there

SomeFunctionNeedingAPointer(&payloadrotatex);

Then the variable will be auto-cleaned up when it goes out of scope.

This will not work if Orbiter core stores that pointer that you give it somewhere.

That is probably why Orbiter is CTDing. Orbiter uses that pointer that you pass it internally; e.g. it tries to access it after your function returns. If you delete it at the end of the function, then Orbiter will CTD when it tries to deference it.

In this case, because Orbiter works with it after your function end, it is Orbiter's job to clean up.

This occurs elsewhere, for example in oapiRegisterModule, you aren't supposed to delete the pointer you pass it, Orbiter takes care of it automatically.

 

[Urwumpe]

Note that for the animations to work, your structures defining the animation have to stay available to Orbiter as long as the animation exists. So deleting the structures at the end of a function is likely far too soon, causing the CTD later, when Orbiter tries to access memory no longer allocated to that structure.

The bug would also happen if you create the variable on the stack then - the variable is only valid during the function.

The best place for such variables is your VESSEL class, though you can also survive well by making them static variables in the function, which are only initialized once. In case of animation components, it does not matter much, because they are not depending on a VESSEL object or mesh.

 

[fred18]

a quick question on the topic:

if I declare a pointer in the class I don't have to delete it right? I'll have to delete only the declarations made with "new" is that correct? Will the pointer declared with the class delete itself when class is destroyed?

 

[Lisias]

a quick question on the topic:

if I declare a pointer in the class I don't have to delete it right? I'll have to delete only the declarations made with "new" is that correct? Will the pointer declared with the class delete itself when class is destroyed?

No.

As a rule of thumb*, the guy that issued the "new" on a class is responsible for issuing the delete on the object he got.

If you issue a new on a class, save the pointer inside another object, and then destroy this last object, the pointer to that former object will be lose and you will have a memory (and potentially resource) leak.

There's a thing called "Smart Pointers" that can help you achieve what you want (a somewhat Java style memory management).


* There're exceptions - some data structures may be told to delete all the objects they hold when deleting themselves - what can lead to problems too: if you have a copy of that pointer in another class, that other pointer is now invalid and will CTD if used.

 

[jedidia]

I get a CTD.

How can I do it safely?

You're supposed to use the destructors integrated into Orbiter. Otherwise, you're messing around in memory Orbiter is actively using without telling it:

bool VESSEL:elAnimationComponent ( UINT anim,
ANIMATIONCOMPONENT_HANDLE hAC
)

and for the animation:

bool VESSEL:elAnimation ( UINT anim ) const

Also note that you have to delete animation components in a certain order: If you delete a parent before a child, there will be trouble!

As a rule of thumb*, the guy that issued the "new" on a class is responsible for issuing the delete on the object he got.

A good rule of thumb, but it breaks down when you're passing "ownership" (not a strictly defined concept in C++, I know*), as is the case here.
EDIT: oh, you actually mention that at the end of your post. Never mind.

*In fact, I'm still a little confused as to who owns the pointer to the mesh group array you're passing to an animation component... :shifty:

 

[Lisias]

Also note that you have to delete animation components in a certain order: If you delete a parent before a child, there will be trouble!

I commonly (but granted, not always) avoid trouble by "deleteing" things in the inverse order I "newed" them.

Not an easy task when we are dealing with Inversion of Control ("they" call us, instead of we calling "them" - essentially, the clbk* functions from API).

Everything I create on the Constructor, I destroy on the Destructor.

What I create on the clbkVisualCreated, I destroy on the clbkVisualDestroyed, and so on.

Things became messy on clbkSetClassCaps (i destroy them on Destructor, before destroying what I made on the Constructor) and mainly on clbkLoadVC, clbkLoadGenericCockpit and clbkLoadPanel. I wish there were "unload" versions of these callbacks to make things easier.

I try as hell to avoid creating things on Pre and PostStep and the other event handlers callbacks. Usually a receipt for memory leaking.

A good place to create and destroy things only needed when you are controlling your vessel is clbkFocusChanged . When getfocus is true and hNewVessel is equal to your vessel handle, you create your little world. When gotfocus is false, and hOldVessel is equal to your vessel handle, you destroy your little world. Not always a good idea, as it can cause delays when switching vessels.

Memory Management is like gray hair: bad with, worst without.

 

[fred18]

Thanks guys.

Those points are quite clear and your explanations are even more clear! What I was actually wondering and I am not sure if i properly understand the answers is simply:

if I do in the header file:

class thisclass{
...
...

anotherclass *ptr

...
...
}

do I have to delete ptr somewhere or not?

 

[Hielor]

Thanks guys.

Those points are quite clear and your explanations are even more clear! What I was actually wondering and I am not sure if i properly understand the answers is simply:

if I do in the header file:

class thisclass{
...
...

anotherclass *ptr

...
...
}

do I have to delete ptr somewhere or not?

That depends on where you actually get the value of ptr from.

 

[jedidia]

simply declaring a pointer does not allocate memory. Memory gets allocated when you use the new keyword. And ideally, the one doing the new should always do the delete, but as mentioned above that isn't always the case.

Calling delete on a pointer you have not allocated ("newed") yourself is usually disastrous, though. It can be assumed that whoever allocated the memory is still using it.

 

[BruceJohnJennerLawso]

And ideally, the one doing the new should always do the delete, but as mentioned above that isn't always the case.

How does that work out when that isnt the case?

 

[Urwumpe]

How does that work out when that isnt the case?

You need a clear definition of the responsibilities, usually by convention ("The calling process has to free the memory allocated for the results of this function").


It is ALWAYS better to avoid such passing of responsibilities - for obvious reasons: Compilers and run-time environments don't read programmer manuals. For example, you can create special pool objects which allocate special objects with new and deletes them if they are no longer needed or when the pool is deleted itself.

Or you use the unique_ptr (or for VC2010 or earlier: auto_ptr class) from the STL.

http://www.cplusplus.com/reference/memory/unique_ptr/
http://www.cplusplus.com/reference/memory/auto_ptr/

 

[jedidia]

How does that work out when that isnt the case?

Then it should be clearly stated in the documentation. Something like object *myptr: a pointer to an object this function needs. The object will be deallocated at the end of the process.

The orbiter documentation is a bit lacking on that point at times, but no documentation is ever complete...

 

[Lisias]

I'm using a (apparently) interesting approach for the problem.

I need to keep track of a lot of objects for a vessel. Sometimes the objects are not needed (VC cockpit) and can be deleted for saving resources (mainly, processor time), but should be recreated after.

So, my constructors have only one parameters: the container handle or a Info struct where the parameters are.

MyVessel::MyVessel()
	: controller(new Controller(getHandle())
{
}

void MyVessel::cblkSetVesselCaps() // or something like that
{
	CrewInfo info;

	info.parm1 = "blah";
	info.parm2 = 1919;

	info.parm3 = E_TYPE_CAPT;
	this->controller.Register(info);

	info.parm3 = E_TYPE_SCI;
	this->sci = this->controller.Register(info);

	// and so on
}

I'm using "this->" to make it clear what are local variables and what are instance's properties.

The Register function is defined as "void Register(ControlInfo const &info)", and is overloaded for each kind "managed class" (each class has its Info struct);

For objects that are "static", i.e., they are to be keep alive while the vessel is alive, the Register feeds the struct into the Constructor, put the object in a "destruction time destroy list", and returns the pointer.

Things became interesting on objetcs that must be kept alive only in certain circustances, as when the vessel has the focus:

class Controller {
	void RegisterForFocus(FocusObjectInfo const & info, UINT const oid);
	operator FocusObject*() {return this->focusObjectArray.data();};
	void HandleFocusGained();
	void HandleFocusLost();

}

void MyVessel::ExampleOfUse() {
	FocusObjectInfo info = {blablabla, blabalbla};
	this->controller.RegisterForFocus(info, OID_SOME_RANDOM_OBJECT);

	FocusObject* fo = controller;
	fo[OID_SOME_RANDOM_OBJECT].doSomething(with, this);
}

Again, the Register will be overloaded for every different class.

The Register will make a *COPY* of the info struct, and keep it in list<FocusObjectInfo>. It's important to make a copy, as this parameter will be commonly came from a stack memory. It will not return any pointer, as the object will be created only when the vessel gets the Focus.

The operator <type>() was created to get access to the current instance of the object (as it is constantly destroyed and recreated). You must %define a OID for each instance of your objects. I made no provisions for arrays of objects. See below how to use this (clever trick, uh?)

So, by monitoring , you :

void MyVessel::clbkFocusChanged(bool getfocus, OBJHANDLE hNewVessel, OBJHANDLE hOldVessel) {
	if (getfocus || hNewVessel == this->GetHandle()) this->controller.HandleFocusGained();
	else if (!getfocus || hOldVessel == this->GetHandle()) this->controller.HandleFocusLost();

	YourRemainingStuff();
}

And you can destroy and recreate everything easily.

That operator trick is kinky, but it works. Below a example made in VS2010 (it compiles and works as expected!)

// Tests.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"

#include <string>
#include <array>

class Controller {
private:
	std::array<std::string,10> stringArray;
	std::array<int,10> intArray;
public:
	void Register(std::string const & info, int const oid)
	{
		stringArray[oid] = info;
	};
	operator std::string*() {return this->stringArray.data();};
	void Register(int const & info, int const oid)
	{
		intArray[oid] = info;
	};
	operator int*() {return this->intArray.data();};
};

	
int _tmain(int argc, _TCHAR* argv[])
{
	Controller c;
	c.Register("000", 0);
	c.Register("001", 1);
	c.Register(1000, 0);
	c.Register(50000, 1);

	std::string *s = c;
	int *i = c;
	printf("%s %s\n", s[0].c_str(), s[1].c_str());
	printf("%d %d\n", i[0], i[1]);
}

 

[kamaz]

Thanks guys.

Those points are quite clear and your explanations are even more clear! What I was actually wondering and I am not sure if i properly understand the answers is simply:

if I do in the header file:

class thisclass{
...
...

anotherclass *ptr

...
...
}

do I have to delete ptr somewhere or not?

There three typical scenarios:

Scenario #1 - the target of ptr does not exist when thisclass is created, and nobody outside thisclass needs it, therefore it should get destroyed together with thisclass

thisclass::thisclass() 
{
   ptr = new anotherclass();
}

thisclass::~thisclass()
{
   delete ptr;
}

Scenario #2 - the target of ptr exists when thisclass is created, and it should continue existing after thisclass is destroyed

anotherclass anotherobject;

thisclass::thisclass() 
{
   ptr = &anotherobject;
}

thisclass::~thisclass()
{
   // note no delete
}

Scenario #3 - the producer-consumer pattern. In this pattern, the target of ptr is created and then passed to another object to process and dispose after processing.

consumer *c;

thisclass::thisclass() 
{
   ptr = new anotherclass();
   c->consume(ptr);
}

thisclass::~thisclass()
{
   // note no delete
}

void consumer::consume(anotherclass *ptr)
{
  ptr->DoSomething();
  delete ptr;
}

In such case however, the calling object should no longer manipulate the target object once it has been passed to consumer. Consequently, there is no need to store pointer to the target object at all. An OOP purist will therefore write:

thisclass::thisclass() 
{
   c->consume(new anotherclass());
}

Or, in case target must be processed in steps:

thisclass::thisclass() 
{
   ptr = new anotherclass();
}

void thisclass::process() 
{
   c->consume(ptr);
   ptr = NULL;
   // crash if trying to access ptr which has been passed to consumer
}

 

[BruceJohnJennerLawso]

Then it should be clearly stated in the documentation. Something like object *myptr: a pointer to an object this function needs. The object will be deallocated at the end of the process.

As in a case where You allocate a block of memory using a pointer, pass it to a function, the function plays with it, then once its finished with it, you deallocate it, right? Thats not that crazy.

What I thought you were referring to was a case where one actor news some memory, then another actor is responsible for deallocating it (ie if the main Orbiter executable were to allocate a block of memory, and an external DLL needed to clean it up for some godawful reason). That sounds like a fast-track to insanity, as far as I am concerned. :lol:

 

[fred18]

There three typical scenarios:

Scenario #1 - the target of ptr does not exist when thisclass is created, and nobody outside thisclass needs it, therefore it should get destroyed together with thisclass

thisclass::thisclass() 
{
   ptr = new anotherclass();
}

thisclass::~thisclass()
{
   delete ptr;
}

Scenario #2 - the target of ptr exists when thisclass is created, and it should continue existing after thisclass is destroyed

anotherclass anotherobject;

thisclass::thisclass() 
{
   ptr = &anotherobject;
}

thisclass::~thisclass()
{
   // note no delete
}

Scenario #3 - the producer-consumer pattern. In this pattern, the target of ptr is created and then passed to another object to process and dispose after processing.

consumer *c;

thisclass::thisclass() 
{
   ptr = new anotherclass();
   c->consume(ptr);
}

thisclass::~thisclass()
{
   // note no delete
}

void consumer::consume(anotherclass *ptr)
{
  ptr->DoSomething();
  delete ptr;
}

In such case however, the calling object should no longer manipulate the target object once it has been passed to consumer. Consequently, there is no need to store pointer to the target object at all. An OOP purist will therefore write:

thisclass::thisclass() 
{
   c->consume(new anotherclass());
}

Or, in case target must be processed in steps:

thisclass::thisclass() 
{
   ptr = new anotherclass();
}

void thisclass::process() 
{
   c->consume(ptr);
   ptr = NULL;
   // crash if trying to access ptr which has been passed to consumer
}

Thanks!

My case is simply this:

myclass{
myclass(anotherclass* _ptr);
...
anotherclass *ptr;
...
};



myclass::myclass(anotherclass* _ptr)
{
  ptr=_ptr;
}

myclass::~myclass(){}

so I can us then ptr-> and get everything I need.

my class is called from the other with

myclassname = new myclass(this);

and then deleted with

delete myclassname;

 

[jedidia]

What I thought you were referring to was a case where one actor news some memory, then another actor is responsible for deallocating it

That is exactly what I was refering to, though the situation isn't as bad as you describe.

Some functions will ask you for a pointer to an object you have to allocate yourself, and then delete it themselves when they're done. Or, like Animations in orbiter, the object receiving the pointer keeps using it, and expects you to use a destructor function to destroy the object so it knows that it now shouldn't use it anymore.