8. Remember that an exception in the destructor is dangerous

This issue was found in LibreOffice project. The error is detected by the following PVS-Studio diagnostic: V509 The 'dynamic_cast<T&>' operator should be located inside the try..catch block, as it could potentially generate an exception. Raising exception inside the destructor is illegal.

virtual ~LazyFieldmarkDeleter()
{
  dynamic_cast<Fieldmark&>
    (*m_pFieldmark.get()).ReleaseDoc(m_pDoc);
}

Explanation

When an exception is thrown in a program, the stack begins to unroll, and objects get destroyed by calling their destructors. If the destructor of an object being destroyed during stack unrolling throws another exception which leaves the destructor, the C++ library will immediately terminate the program by calling the terminate() function. What follows from this is the rule that destructors should never let exceptions out. An exception thrown inside a destructor must be handled inside the same destructor.

The code cited above is rather dangerous. The dynamic_cast operator will generate a std::bad_cast exception if it fails to cast an object reference to the required type.

Likewise, any other construct that can throw an exception is dangerous. For example, it's not safe to use the new operator to allocate memory in the destructor. If it fails, it will throw a std::bad_alloc exception.

Correct code:

The code can be fixed using the dynamic_cast not with a reference, but with the pointer. In this case, if it's impossible to convert the type of the object, it won't generate an exception, but will return nullptr.

virtual ~LazyFieldmarkDeleter()
{
  auto p = dynamic_cast<Fieldmark*>m_pFieldmark.get();
  if (p)
    p->ReleaseDoc(m_pDoc);
}

Recommendation

Make your destructors as simple as possible. Destructors aren't meant for memory allocation and file reading.

Of course, it's not always possible to make destructors simple, but I believe we should try to reach that. Besides that, a destructor being complex is generally a sign of a poor class design, and ill-conceived solutions.

The more code you have in your destructor, the harder it is to provide for all possible issues. It makes it harder to tell which code fragment can or cannot throw an exception.

If there is some chance that an exception may occur, a good solution is usually to suppress it by using the catch(...):

virtual ~LazyFieldmarkDeleter()
{
  try 
  {
    dynamic_cast<Fieldmark&>
      (*m_pFieldmark.get()).ReleaseDoc(m_pDoc);
  }
  catch (...)
  {
    assert(false);
  }
}

True, using it may conceal some error in the destructor, but it may also help the application to run more stably in general.

I'm not insisting on configuring destructors to never throw exceptions - it all depends on the particular situation. Sometimes it's rather useful to generate an exception in the destructor. I have seen that in specialized classes, but these were rare cases. These classes are designed in such a way that the objects generate an exception upon the destruction, but if it is a usual class like "own string","dot", "brush" "triangle", "document" and so on, in these cases the exceptions shouldn't be thrown from the destructor.

Just remember that double exception on end cause a program termination, so it's up to you to decide if you want this to happen in your project or not.