Tuning C++ Destructors, Part II

Last time I posted a snipped of code that I examined as part of my GDC talk. This is an imperfect destructor in many ways. How many can you detect?
   if( m_pOrdnance != NULL )
      delete m_pOrdnance;
      m_pOrdnance = NULL;
   if( m_hAlert != NULL )
      m_hAlert = NULL;
   for( int i = 0; i < m_CrewSize; i++ )
      delete m_Crew[i];
Potential Problem #1: Calling a function within a destructor. In the absolute worst case, Unload() is a pure virtual function, and the call invokes undefined behavior. Even if Unload() is not virtual, it may be doing unnecessary work that can have a performance impact. If you’re writing a game and this destructor is being called hundreds of times per frame, Unload() may be overkill that’s worth avoiding.
Potential Problem #2: Calling a function within a destructor that could throw an exception. Destructors must never throw.
Potential Problem #3: Calling a member function to do clean up. Rule of thumb: any class should clean up after itself. That’s what destructors are for. The destructors for subobjects are called automatically after the destructor body is executed. Forcing users of a class to invoke a clean-up function is error-prone and unnecessary. It’s more likely that the .Empty() function is doing unnecessary work and can be eliminated completely.
Real Problem #4: Checking for NULL before calling delete. The C++ Standard guarantees that delete will correctly handle NULL pointers. Checking for NULL is redundant work that should be avoided.
Real Problem #5: Clearing out pointers in destructors. The Enterprise object is on the verge of utter destruction. Upon exit from this code, any access to the memory previously occupied by the Enterprise object invokes undefined behavior. True, you may be more likely to catch stray pointer issues by setting memory locations to known values, but clearing pointers (or any other memory) doesn’t buy you absolute or true protection. Furthermore, it can adversely affect performance in common destructors. Set memory to known values in debug builds only.
Potential Problem #6: Clearing out memory in destructors. The check for NULL prior to CloseHandle is valid and must remain. However, setting the handle to NULL buys you nothing but a false sense of security. See Real Problem #4. There is at least one case where clearing memory in destructors could be worthwhile – when you need to clear out an encryption key or other important game data from memory. My hackers will attempt to access or update critical values in memory, so clearing those values can be important.
[Note: some handles are properly compared against NULL and others are properly compared against INVALID_HANDLE_VALUE. Understand the API you’re using and what type of handle it expects and returns.]
Potential Problem #7: Freeing up pointers in a container. m_Crew contains a list of raw pointers to memory. The destructor is correctly cleaning up those pointers. However, there’s a better way: shared_ptr. If the m_Crew container owned a list of shared_ptrs, the shared_ptr destructors would automatically handle the cleanup. Furthermore, shared_ptr would allow for other lists and objects to maintain references to the pointers, guaranteeing no orphan pointers to garbage.
With the changes proposed above, here’s what the destructor would look like:
   delete m_pOrdnance;
   if( m_hAlert != NULL )
That’s good, but we can do even better. Let’s take the shared_ptr advice and apply it to the m_pOrdnance pointer. We can also create a little wrapper class around HANDLE objects that performs the same sort of work as shared_ptr:
class Handle
   HANDLE m_Handle;
   Handle() : m_Handle( NULL ) {}
   explicit Handle( HANDLE h ) : m_Handle(h) {}
   ~Handle() { if (m_Handle != NULL ) CloseHandle( m_Handle ); }
Now we have perfection, the empty destructor:
All Enterprise subobjects are owned resources that automatically clean up after themselves. Not only is our code more readable and maintainable, it’s also more likely to be exception safe.
[Note: One commenter wrote that we should make the Enterprise destructor virtual, “just in case.” The Enterprise destructor should be virtual if and only if one could ever call “delete pEnterprise” when pEnterprise was actually pointing to a class derived from Enterprise. A good rule of thumb is to make the destructor virtual if there are other virtual functions in the Enterprise class.]
This entry was posted in C++. Bookmark the permalink.

8 Responses to Tuning C++ Destructors, Part II

  1. Unknown says:

    Suggesting to clear out pointers in debug builds may lead to code that hides bugs in debug but crashes in release builds. That’s probably not what you’d want. Other than that, great advice!

  2. Pete says:

    The idea behind clearing pointers in debug builds is to expose bugs, not hide them. Clearing pointers in release builds *may* help prevent bugs, but it’s more likely to mask them. Worse, clearing pointers in release builds affects performance every time the destructor is called. For common objects in games, that’s a cost that may not be worth the tradeoff.

  3. Leonid says:

    I’m about potential problem #1.even if Unload() is virtual and it is overloaded, in destructor will be called Enterprise::Unload() not subclass::Unload(),in other case, if Unload() is abstract – you will get compiler error. Where did you find undefined behavour?You should learn C++ more, I think.

  4. João says:

    I have been waiting for this follow-up, and it was a good read, thanks!

  5. Pete says:

    A clarification regarding Potential Problem #1. In the absolute worst case, Unload() is a *pure* virtual function, and the call invokes undefined behavior (C++0X Standard 10.4/6: "the effect of making a virtual call to a pure virtual function for the object being created (or destroyed) from a constructor (or destructor) is undefined.") If Unload() is virtual — but not pure — the call invokes Enterprise::Unload(), which is probably not what the programmer intended. If it is what the programmer intended, the code should call it out specifically, i.e. Enterprise::Unload().

  6. Roman says:

    How would you go about implementing a destructor for an object that also has to have some kind of destroy mechanism? Would you duplicate the code, omitting the "bad" code in the destructor? As an over-simplification: are problems #5 and #6 severe enough to justify _not_ calling Empty() in CString’s d’tor?

  7. Michael says:

    As ‘no name’ mentioned, clearing pointers to NULL in a debug build but not the release build is about the worst thing you can do, as it will cause bugs to be avoided in the debug build that will then manifest in the release build (since NULL gets checked before using the pointer, a least in the dtor example – likely in other bits of code, too). Better to have the debug build set the pointer to something certain to be invalid (like 0xdeadbeef or something) so the debug build will ‘crash fast’ if the pointer is somehow used after being freed.Also, if the pointers being allocated by the object aren’t shared wth anything else, then using unique_ptr (or boost::scoped_ptr if unique_ptr isn’t available to you yet) might be a somewhat better choice than shared_ptr as there’s a tiny bit less overhead – no ref count to maintain, though that’s really not too much of an issue if you’re never sharing the pointer, since the ref count won’t be manipulated except a couple times (but then again, you were concerned about the overhead of setting pointers to NULL). More importantly, unique_ptr (or scoped_ptr) will help enforce and document the fact that those resources are solely owned by that object.

  8. Michael says:

    By the way – nice article.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )


Connecting to %s