29 replies to this topic

[simpler]

Hey I have bumped into a lot of code that uses a Cleanup() member function for their classes instead of using the destructor. I have found myself starting to mix both alternatives which feels bad and I'd rather want to be consistent. What are the pros and cons? What's your opinion?

EDIT: The same question goes for the constructor and Init().

Edited by simpler, 12 October 2012 - 07:04 AM.

[swiftcoder]

There is almost never a good reason for a separate 'cleanup' function in C++. You are stuck with that sort of thing in a garbage collected language (i.e C#'s IDisposable), but the careful use of RAII renders manual collection superfluous in C++.

[IgnatusZuk]

I usually use a Cleanup method in languages that don't use destructors.
But with C++ it's better to just stick with constructors and destructors, what swiftcoder said.

[simpler]

Thanks for the answers! I know what I'm going to use from now on.

[SiCrane]

There are occasionally reasons to separate construction and initialization or destruction and clean up, most of which involve the fact that constructors and destructors are limited in their ability to convey error information. On platforms with poor exception handling, such as some consoles and embedded systems, or if you need to do a significant amount of interop with languages that don't understand C++ exceptions, then you may be in a situation where you want to do the heavy lifting in separate functions.

[frob]

It also makes sense for reusable objects, especially heavyweight objects that are expensive to create and destroy, but cheap to recycle.

An example of reusable objects would be the C++ file streams. You can use it once by create, open, close, then destroy; or reuse with create, open, close, open, close, open, close, ... etc., then destroy.

[swiftcoder]

It also makes sense for reusable objects, especially heavyweight objects that are expensive to create and destroy, but cheap to recycle.

Let me rephrase my earlier statement to read: "One should never aim to design code that requires a cleanup function in C++".

If you have an object which should be reused, or which lifetime should exceed its use, then consider allowing it to provide an RAII-compliant 'handle' object, which encapsulates the short-term operation.

An example of reusable objects would be the C++ file streams. You can use it once by create, open, close, then destroy; or reuse with create, open, close, open, close, open, close, ... etc., then destroy.

Are fstreams really that heavyweight? Glancing at my local header files it looks like a mutex, a buffer, an underlying stream, and a handful of state variables. It seems to me that the cost of actually locking the mutex, and invoking an open syscall should dominate the construction costs of allocating the buffer.

[frob]

An example of reusable objects would be the C++ file streams. You can use it once by create, open, close, then destroy; or reuse with create, open, close, open, close, open, close, ... etc., then destroy.

Are fstreams really that heavyweight? Glancing at my local header files it looks like a mutex, a buffer, an underlying stream, and a handful of state variables. It seems to me that the cost of actually locking the mutex, and invoking an open syscall should dominate the construction costs of allocating the buffer.

Please reread my post. No, they are not heavyweight. They are reusable.

[Bregma]

Please reread my post. No, they are not heavyweight. They are reusable.

Sure, they're reusable, but the heavy cost is in the open operation, not the object creation operation (except when the object creation includes the open operation).

Reusing an fstream is like reusing an integer variable: the cost to maintainabilty heavily outweighs the runtime cost savings.

[frob]

Please reread my post. No, they are not heavyweight. They are reusable.

Sure, they're reusable, but the heavy cost is in the open operation, not the object creation operation (except when the object creation includes the open operation).

Reusing an fstream is like reusing an integer variable: the cost to maintainabilty heavily outweighs the runtime cost savings.

The point was not about reuse cost.

The point was that sometimes a "reset this object" function is a good thing.

[Servant of the Lord]

Another (unfortunate) reason for needing a Cleanup() or Initialize() function is from poor third-party API designs that you have to work around in your own code.

It also makes sense for reusable objects, especially heavyweight objects that are expensive to create and destroy, but cheap to recycle.

An example of reusable objects would be the C++ file streams. You can use it once by create, open, close, then destroy; or reuse with create, open, close, open, close, open, close, ... etc., then destroy.

And conveniently, (some of) the constructors call open(), and the destructors call close() for you, so actual use can look like: create, destroy, or create, open, destroy, or create, close, open, destroy. Very nice design - very flexible.

Another benefit of that, is it makes the destructor and constructor implementation cleaner, especially when you have multiple constructors (and don't yet have constructor chaining).

MyFile::MyFile()
{

}

MyFile::MyFile(filename)
{
	 this->Open(filename);
}

My::MyFile(data)
{
	 this->OpenFromData(data);
}

MyFile::Open(filename)
{
	 data = ::LoadFileData(filename);
	 this->OpenFromData(data);
}

MyFile::OpenFromData(data)
{
	 //actual loading from data...
}

Edit: Bah, code boxes messing up.

Edited by Servant of the Lord, 12 October 2012 - 11:51 AM.

[swiftcoder]

The point was that sometimes a "reset this object" function is a good thing.

And that's precisely the point I am questioning.

If the object had steep construction cost, then by all means, (carefully) provide reset/reuse functionality. But that is the only case in which I see it as a good thing.

[Cornstalks]

A manual cleanup() function is okay so long as it also automatically calls this in the destructor. If I'm not required to call it for resources to be released and they will be done so automatically when the object is destroyed, I have no problem with it, and in some cases it might be nice to be able to reset/reuse the object, in which case the cleanup() function is ok.

For example, in C++11 I can do vector.clear(); vector.shrink_to_fit(); to destroy all the objects in the vector and clear its allocated memory. However, I'm not required to do this, as the destructor will do the same thing for me too.

[lride]

If you have global objects that requires other objects to be initialized or destroyed before initializing or destroying themselves, init() cleanUp() comes in handy.

[source lang="cpp"]MemoryManager memoryManager; //memoryManager must be initialized before other objectsRenderManager renderManager; //but c++ doesn't guarantee that memoryManager getsAIManager aiManager; //initialized in the order it appears in the source code[/source]
In this case we can make constructors and destructors do nothing and put init() and cleanUp() instead to ensure the order.
[source lang="cpp"]MemoryManager memoryManager;RenderManager renderManager;AIManager aiManager;// constructors do nothing hereint main(){ memoryManager.init(); renderManager.init(); aiManager.init();//Game Running...............//game over-shut things down aiManager.cleanUp(); renderManager.cleanUp(); memoryManager.cleanUp();return 0;}[/source]

Edited by lride, 12 October 2012 - 01:05 PM.

[MrDaaark]

Re: Constructor vs Init()

Because sometimes they are 2 different concepts.

A lot of my game objects just call a Reset()/Init() function in their constructor because I will reset them to an initial state over the course of their lifetime. A lot of times because there is a fixed number of them, and they disappear and reappear on a frequent basis. By calling Reset, I effectively create a new one, instead of allocating and de-allocating a new instance of an object for nothing. Also, sometimes I need to clear out references and other things that are no longer valid. A re-used object doesn't have a last attacker or a target yet.

I've also used it for particles. When a particle dies, it gets Reset(). Which means it returns to the origin, and then re-assigns itself some random properties as if it was a new instance.

Edited by Daaark, 12 October 2012 - 01:07 PM.

[swiftcoder]

If you have global objects that requires other objects to be initialized or destroyed before initializing or destroying themselves, init() cleanUp() comes in handy.

I have two issues with this:
a) You should most likely not be using global variables (and as a bonus, the order of destruction for member variables is well defined).
b) You should not have hidden dependencies between objects. If object B requires object A to be allocated first, then for goodness sake enforce that invariant in code.

[SiCrane]

[source lang="cpp"]MemoryManager memoryManager; //memoryManager must be initialized before other objectsRenderManager renderManager; //but c++ doesn't guarantee that memoryManager getsAIManager aiManager; //initialized in the order it appears in the source code[/source]

Actually, C++ does guarantee that global variables defined in the same translation unit are constructed in order of definition.

[Bregma]

I've been developing software a long time. A long time. I've seen a lot of trends come and go, and been amazed by and adopted the better ideas over the decades.

One of the ones that works well, really well, is the idea of class invariants. They're facts about objects of the class that are always true (except perhaps during the execution of a member function body). To have class invariants, you need to construct your objects in the constructor, and destroy them in the destructor. Period.

Now, I'm sure all you all are really clever and probably write genius-level code, so you don't have to be able to reason about code you're written 6 weeks after you wrote it. Me, I appreciate the fact that I can rely on those good old class invariants to hold true whenever I see an object lying around. So, go ahead and partially construct objects and then initialize them later, and maybe clean them up and leave them lying around before you destroy them, and don't bother with validity checks on each and every access because, providence knows, you never write buggy code. But when you've just spent 12 hours trying to track down that crasher the day before release and you finally find the problem, do think of me. Don't think this is a real life problem? Consider a std::istream, how it does not have a proper class invariant, and how many questions in these very forums are spawned by people not properly checking the object state before trying to use it.

Of course, there's nothing wrong with a reset function. Just so long as those class invariants don't get violated.

[L. Spiro]

frob and Cornstalks gave the correct answers.

Here is an excerpt from my engine. A basic std::vector class:
The constructor:

LSE_CALL					CSVectorCrtp() :
			m_tLen( 0 ),
			m_tAllocated( 0 ),
			m_ptData( NULL ) {
		}

The Reset function:

/**
		 * Reset the list completely.  Frees all memory.
		 */
		LSVOID LSE_CALL				Reset() {
			for ( LSINT32 I = static_cast<LSINT32>(m_tLen); --I >= 0; ) {
				Destroy( static_cast<_tDataType>(I) );
			}
			m_paDefaultAllocator->Free( m_ptData );
			m_ptData = NULL;
			m_tLen = m_tAllocated = 0;
		}

The destructor:

LSE_CALL					~CSVectorCrtp() {
			Reset();
		}

There is no harm done in the fact that it can easily be reused. Anyone who argues otherwise should swiftly be ignored.
It can obviously be used on a normal one-off basis, as is standard, but it is such a general-purpose class that who is to say it will never be useful to reuse the same object?
I personally do it all the time. It is just another option for our convenience and control.

Speaking of control:

/**
		 * Reset the list without deallocating the whole buffer.
		 */
		LSVOID LSE_CALL				ResetNoDealloc() {
			for ( LSINT32 I = static_cast<LSINT32>(m_tLen); --I >= 0; ) {
				Destroy( static_cast<_tDataType>(I) );
			}
			m_tLen = 0;
		}

While there are ways to control std::vector to achieve the same results, they are fairly non-intuitive. It is a lot easier to just use Reset() and ResetNoDealloc(),which have obvious meanings.


Reusable classes are never a bad thing. They are just another layer of options, and that is always a good thing.


L. Spiro

Edited by L. Spiro, 13 October 2012 - 06:29 AM.

[JohnnyCode]

yes, from C# object gets anownced that it would like to be freed (By OS manager GC). You then perform alll logic to free resources the object posesss. Like sicrane said, study cli/c++

in c++ destrocturos are not necesary, nor any good logic, from C#, managememet of memory yes

You can still design your logic to not need destructors, but your logic must be freed and managed well