In this article, we'll look at how to correctly destroy objects in the OOP-based C++ program without redundant operations. This is the final article in the series about the bugs in qdEngine.

Failed resource release in qdEngine code

Here's a list of previous articles about checking the qdEngine game engine:

1. Let's check the qdEngine game engine, part one: top 10 warnings issued by PVS-Studio
2. Let's check the qdEngine game engine, part two: simplifying C++ code
3. Let's check the qdEngine game engine, part three: 10 more bugs

Once I wrote them, I still had one more interesting PVS-Studio warning. So, I decided to make a separate article for this. Here's the warning:

V1053 [CERT-OOP50-CPP] Calling the 'Finit' virtual function in the destructor may lead to unexpected result at runtime. gr_dispatcher.cpp 54

We can call virtual functions in destructors, and the C++ standard clearly describes how this works. Unfortunately, such code is a magnet for errors, that's why many coding standards and analyzers recommend against using these calls. I once wrote an article on this topic: "Virtual function calls in constructors and destructors (C++)". If you're a beginner in C++ or want to refresh your memory, I suggest taking a peek at it before you continue reading. I also encourage you to read it if you're not sure what we're talking about.

The code fragment related to the warning is quite large, but you can safely skip it. We'll break it down below using some synthetic examples.

The Finit function is called in the base class destructor. Since the DDraw_grDispatcher subclass has already been destroyed, its Finit function isn't called.

class grDispatcher
{
  ....
  virtual ~grDispatcher();
  virtual bool Finit();
  ....
};

grDispatcher::~grDispatcher()
{
  Finit();
  if (dispatcher_ptr_ == this) dispatcher_ptr_ = 0;
}

bool grDispatcher::Finit()
{
#ifdef _GR_ENABLE_ZBUFFER
  free_zbuffer();
#endif

  flags &= ~GR_INITED;
  SizeX = SizeY = 0;
  wndPosX = wndPosY = 0;
  screenBuf = NULL;
  delete  yTable;
  yTable = NULL;

  return true;
}

class DDraw_grDispatcher : public grDispatcher
{
  ....
  ~DDraw_grDispatcher();
  bool Finit();
  ....
};

DDraw_grDispatcher::~DDraw_grDispatcher()
{
  if (ddobj_)
  {
    ddobj_ -> Release();
    ddobj_ = NULL;
  }

  video_modes_.clear();
}

bool DDraw_grDispatcher::Finit()
{
  grDispatcher::Finit();

  if (back_surface_)
  {
    while(
      back_surface_ -> GetBltStatus(DDGBS_ISBLTDONE) == DDERR_WASSTILLDRAWING);
    back_surface_ -> Unlock(&back_surface_obj_);
    ddobj_ -> SetCooperativeLevel((HWND)Get_hWnd(),DDSCL_NORMAL);
    if (fullscreen_ && ddobj_) ddobj_ -> RestoreDisplayMode();
  }

  if (prim_surface_)
  {
    prim_surface_ -> Release();
    prim_surface_ = NULL;
  }

  if (back_surface_)
  {
    back_surface_ -> Release();
    back_surface_ = NULL;
  }

  return true;
}

Synthetic code example for error analysis

Now, let's figure out what the issue is. We'll use the synthetic code and the Compiler Explorer website to quickly explore how the code works.

We need to create a class hierarchy to manage some resources. We'll also use the polymorphism principle to work with objects via a pointer to the base class.

Let's start with the simplest base class:

#include <memory>
#include <iostream>

class Resource
{
public:
  void Create() {}
  void Destroy() {}
};

class A
{
  std::unique_ptr<Resource> m_a;

public:
  void InitA()
  {
    m_a = std::make_unique<Resource>();
    m_a->Create();
  }

  virtual ~A()
  {
    std::cout << "~A()" << std::endl;
    if (m_a != nullptr)
      m_a->Destroy();
  }
};

int main()
{
  std::unique_ptr<A> p = std::make_unique<A>();
  return 0;
}

So far, everything seems OK. We get the following output for the online example:

~A()

Then we find out that the class state should be reset from time to time. We need to release resources and rather than waiting for the destructor call when the class is destroyed. Here's the design error: a virtual function is created to clean up the class. A developer creates an additional virtual class interface like this:

#include <memory>
#include <iostream>

class Resource
{
public:
  void Create() {}
  void Destroy() {}
};

class A
{
  std::unique_ptr<Resource> m_a;

public:
  void InitA()
  {
    m_a = std::make_unique<Resource>();
    m_a->Create();
  }

  virtual void Reset()
  {
    std::cout << "A::Reset()" << std::endl;
    if (m_a != nullptr)
    {
      m_a->Destroy();
      m_a.reset();
    }
  }

  virtual ~A()
  {
    std::cout << "~A()" << std::endl;
    Reset();
  }
};

int main()
{
  std::unique_ptr<A> p = std::make_unique<A>();
  return 0;
}

The online example displays the following output:

~A()
A::Reset()

The virtual Reset function is added to free resources. The destructor doesn't release resources to avoid the code duplication. Now it just calls the function.

So far, it seems like everything is still OK, but let's add the subclass:

#include <memory>
#include <iostream>

class Resource
{
public:
  void Create() {}
  void Destroy() {}
};

class A
{
  std::unique_ptr<Resource> m_a;

public:
  void InitA()
  {
    m_a = std::make_unique<Resource>();
    m_a->Create();
  }

  virtual void Reset()
  {
    std::cout << "A::Reset()" << std::endl;
    if (m_a != nullptr)
    {
      m_a->Destroy();
      m_a.reset();
    }
  }

  virtual ~A()
  {
    std::cout << "~A()" << std::endl;
    Reset();
  }
};

class B : public A
{
  std::unique_ptr<Resource> m_b;

public:
  void InitB()
  {
    m_b = std::make_unique<Resource>();
    m_b->Create();
  }

  void Reset()
  {
    std::cout << "B::Reset()" << std::endl;
    if (m_b != nullptr)
    {
      m_b->Destroy();
      m_b.reset();
    }
    A::Reset();
  }

  ~B()
  {
    std::cout << "~B()" << std::endl;
    Reset();
  }
};

int main()
{
  std::unique_ptr<A> p = std::make_unique<B>();
  p->Reset();
  std::cout << "------------" << std::endl;
  p->InitA();
  return 0;
}

The online example displays the output:

B::Reset()
A::Reset()
------------
~B()
B::Reset()
A::Reset()
~A()
A::Reset()

If we explicitly call the Reset function from the external code, everything works fine. The B::Reset() function is called, and then it calls a function with the same name from the base class.

There is an issue with the destructor. Each destructor calls the Reset function. It results in redundant operations because the Reset function calls its own version from the base class.

If we continue this strange inheritance, we will make the issue worse and worse. And it will cause more and more redundant function calls.

Here's the code output where another class has been added:

C::Reset()
B::Reset()
A::Reset()
------------
~C()
C::Reset()
B::Reset()
A::Reset()
~B()
B::Reset()
A::Reset()
~A()
A::Reset()

There's obviously a class design error. But it's obvious to us now, though. In a real project, such errors can thrive and remain unnoticed by the developer's eye: the code works and the Reset functions do their task. But redundant and inefficient operations are still here.

When a dev notices the described error and tries to fix it, they risk making two other typical errors.

The first option. They declare the Reset functions as non-virtual and do not call the base (x::Reset) options in them. Then, each destructor calls only the Reset function from its class and releases only its own resources. This really takes away the redundancy in the operation of destructors. However, when Reset is called externally, the cleanup of the object state breaks. The broken code displays:

A::Reset()   // Cleanup resources is broken externally
------------
~C()
C::Reset()
~B()
B::Reset()
~A()
A::Reset()

The second option. They call the Reset virtual function once from the base class destructor. This doesn't work because, according to C++ rules, implementation of the Reset function from base class will be called, not from subclasses. This makes sense because by the time the ~A() destructor is called, all subclasses have been destroyed, and functions can't be called from them. The broken code displays:

C::Reset()
B::Reset()
A::Reset()
------------
~C()
~B()
~A()
A::Reset()  // Release resources only in the base class

It's this type of the error that we've found in the qdEngine project thanks to PVS-Studio. If you wish, now you can scroll up to the beginning of the article and see the corresponding code from the game engine.

Fixed synthetic code

So, how can we correctly use classes to avoid numerous redundant calls?

To do this, we need to separate the release of internal class resourses from the public interface. It'd be better to create non-virtual functions that are only responsible for releasing the data in classes where they're declared. Let's name them ResetImpl and make private because they're not for the external use.

Destructors will simply delegate their work to the ResetImpl functions.

The Reset function will remain public and virtual. It'll release data of all classes using the same ResetImpl helper functions.

Let's put everything together and write correct code:

#include <memory>
#include <iostream>

class Resource
{
public:
  void Create() {}
  void Destroy() {}
};

class A
{
  std::unique_ptr<Resource> m_a;

  void ResetImpl()
  {
    std::cout << "A::ResetImpl()" << std::endl;
    if (m_a != nullptr)
    {
      m_a->Destroy();
      m_a.reset();
    }
  }

public:
  void InitA()
  {
    m_a = std::make_unique<Resource>();
    m_a->Create();
  }

  virtual void Reset()
  {
    std::cout << "A::Reset()" << std::endl;
    ResetImpl();
  }

  virtual ~A()
  {
    std::cout << "~A()" << std::endl;
    ResetImpl();
  }
};

class B : public A
{
  std::unique_ptr<Resource> m_b;

  void ResetImpl()
  {
    std::cout << "B::ResetImpl()" << std::endl;
    if (m_b != nullptr)
    {
      m_b->Destroy();
      m_b.reset();
    }
  }

public:
  void InitB()
  {
    m_b = std::make_unique<Resource>();
    m_b->Create();
  }

  virtual void Reset()
  {
    std::cout << "B::Reset()" << std::endl;
    ResetImpl();
    A::Reset();
  }

  virtual ~B()
  {
    std::cout << "~B()" << std::endl;
    ResetImpl();
  }
};

class C : public B
{
  std::unique_ptr<Resource> m_c;

  void ResetImpl()
  {
    std::cout << "C::ResetImpl()" << std::endl;
    if (m_c != nullptr)
    {
      m_c->Destroy();
      m_c.reset();
    }
  }

public:
  void InitC()
  {
    m_c = std::make_unique<Resource>();
    m_c->Create();
  }

  virtual void Reset()
  {
    std::cout << "C::Reset()" << std::endl;
    ResetImpl();
    B::Reset();
  }

  virtual ~C()
  {
    std::cout << "~C()" << std::endl;
    ResetImpl();
  }
};

int main()
{
  std::unique_ptr<A> p = std::make_unique<C>();
  p->Reset();
  std::cout << "------------" << std::endl;
  return 0;
}

The online example displays:

C::Reset()
C::ResetImpl()
B::Reset()
B::ResetImpl()
A::Reset()
A::ResetImpl()
------------
~C()
C::ResetImpl()
~B()
B::ResetImpl()
~A()
A::ResetImpl()

I can't say the synthetic code looks nice. However, it's full of As, Bs, and Cs, so it's very easy to make a typo. Let's forgive the synthetic examples for this. The code works, and we've deleted redundant operations. That's a good result.

Conclusion

A virtual function call in a destructor isn't always an error. However, this may be a sign of the poor class design. The qdEngine project is a great example of such a case.

The PVS-Studio analyzer issues the V1053 warning if a virtual function is called in a constructor or destructor. This is a good reason to take another look at the code and see if there's anything we can fix or refactor.