Tuning C++ Destructors


I’m giving a talk next week at GDC called The Beauty of Destruction. Here’s a snippet of code we’ll be looking at during the talk, probably similar to C++ destructors you’ve observed if you’re a C++ coder. This is an imperfect destructor in many ways. How many can you detect?
 
Enterprise::~Enterprise()
{
   Unload();
   m_ReactorCore.Empty();
   if( m_pOrdnance != NULL )
   {
      delete m_pOrdnance;
      m_pOrdnance = NULL;
   }
   if( m_hAlert != NULL )
   {
      CloseHandle(m_hAlert);
      m_hAlert = NULL;
   }
   for( int i = 0; i < m_CrewSize; i++ )
      delete m_Crew[i];
}
Advertisements
This entry was posted in C++. Bookmark the permalink.

7 Responses to Tuning C++ Destructors

  1. Sebastien says:

    Hmm!Unload() throw an exceptiondelete[] m_Crew;CloseHandle fail (::CloseHandle(m_hEvent); DWORD dwError = GetLastError();)

  2. Evgeny says:

    If this code snippet is all what is given, I don’t see any problems in it. Am I dumb? 😦

  3. Michael says:

    First, "Enterprise" would be an instance. The class name would either NX, Constitution, Excelsior, Ambassador, Galaxy or Sovereign. And that might lead to the first real imperfection: it trolls us Star Trek geeks. ;-)If m_ReactorCore is an STL container, it should be .clear(), not .Empty(). But even if the check-if-empty function were intended, the case is incorrect. Which tells me I’m probably *way* off base there.Additionally, there’s no obvious blocking to wait for all services within the class to finish (i.e. a WaitForSingleObject on an "all clear to destruct" event.) But that could be hidden away in Unload(). For something as complicated as the Enterprise, such an event is probably necessary.Finally, and perhaps this is personal preference, I would either clear the m_CrewSize container after the deletes, pop elements out of the container for each delete, or at least check-if-NULL->delete->set-NULL each element. I disagree with Sebastien’s approach, as I’m pretty sure that won’t work if m_Crew is an std::vector. (And considering your crew size can change during the life time of the class instance, you certainly don’t want a static array. You probably don’t even want std::vector, but would be better served by std::map or even std::set. And both of those can be quickly looped through with iterators.)Of course, if the concern is performance, one could go the other way and not explicitly assign NULL values or empty containers. Even ignoring potential compiler optimizations, though, the performance gains from such would be negligible, and potential risk for buggy code depending on a dangling member pointer would seem reason enough to justify the extra effort.There’s also the possibility that adjusting the size of the containers could have side-effects either in heap activity or custom behavior. But if you’re worrying about the timing of those in a class’s destructor, you’re most likely overthinking things a bit.Also, where are the comments? I’d be particularly interested to know roughly what Unload does in the context of the class. If Unload() is a partial-destructor that may be called elsewhere, then the act of clearing m_ReactorCore may belong in there. Also, I’m not clear on whether it’s safe to clear m_ReactorCore that way; If it contains pointers, those pointers may need to be freed. If it contains pointers that don’t need to be freed (i.e. cached values pointing elsewhere in the program), then a note should be made so that an unfamiliar programmer doesn’t insert a destructor loop leading to double-free circumstances and heap corruption.Finally, return types and exceptions. CloseHandle returns a BOOL. Check it. At the very least, wrap it in a VERIFY. Better if you check it, call GetLastError(), convert it to a string and *then* assert. Better still if you have a logging infrastructure and can log out the error when not in debug. I don’t know if Unload or m_ReactorCore.Empty() have return values to check, but Sebastien’s right that you should be checking for exceptions.I’d suggest using a macro to clean up single-pointer deletes and handle closing. And it’s fairly trivial to write template functions to handle <value type> and <key type, valuetype> stl containers that contain pointers. Both of these approaches can be adjusted to ensure whatever safety or debugging behavior suits your project, and make such easy to change project-wide when the need arises.That’s all I can think of.

  4. João says:

    Make it virtual just in case. Other than that, I’m assuming the original programmer knew what he was doing when he chose to check some values for NULL before deleting and not others. Would be nice with some comments though.

  5. Jean-Sylvestre says:

    most have been cited, but here’s my list, starting by the most obvious:You shouldn’t compare a handle with NULL ( handle 0 may be valid ), use INVALID_HANDLE_VALUE insteadThere’s no need to set m_hAlert to null (and it’s incorrect, use INVALID_HANDLE_VALUE)There’s no need to check if m_pOrdonance is null (delete 0 is ok)There’s no need to set m_pOrdonance to null (what for ?)The following can be discussed (perhaps the programmer had a good reason to do so): Unload may throw (ohnoes!!!)/bitching/ The naming Empty() annoys me, if this method frees data, then it should be called differently /bitching/If Empty() actually releases anything, check if the data is going to be released in m_ReactorCore’s destructor, then Empty() is redundantYou should consider using delete[] m_Crew, or better use a containers like std::vectorIf m_Crew is a container, remove m_CrewSizeAbout m_Crew, consider storing objects instead of pointers, if it is not possible, consider storing smart pointers.If you can’t afford to use std::vector, you may want to use std::array (or boost similar), then you don’t have to worry about m_CrewSizeI wish I could attend the talk!

  6. Jean-Sylvestre says:

    I just got accross this one:Check Unload() is not virtual and that it does not call virtual functions. The behavior of Unload() may be undefined in this case. More information at http://www.artima.com/cppsource/nevercall.html

  7. Michael says:

    @Jean-Sylvestre Setting m_pOrdnance and m_hAlert to sentinel values aids in debugging; If you see sentinel values in your class instance members, it gives you a better idea where that instance is in its lifetime. It also helps as an additional level of safety for thread safety issues; If for some strange reason a thread starts running code through the class member as it’s destructing, it’s less likely to crash if it checks for sentinal values before using class members. Granted, it leads to an open race issue (Unless all your member pointers are volatile and you use InterlockedExchangePtr, but the performance penalty for that is a bit much…) and only helps if your code is already buggy, but it could be the difference between a debug log entry and a core dump, which can be very important.

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 )

Twitter picture

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

Facebook photo

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

Google+ photo

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

Connecting to %s