В этой статье мы рассмотрим, как правильно разрушать объекты в ООП программе на языке C++, не выполняя избыточных операций. Этим мы завершим цикл публикаций, посвящённый обзору ошибок в игровом движке qdEngine.

Неудачная реализация очистки ресурсов в коде qdEngine

Предыдущие статьи о проверке игрового движка qdEngine:

1. Топ 10 предупреждений PVS-Studio.
2. Упрощение C++ кода
3. Дополнительная десятка багов

После этих публикаций у меня осталось ещё одно интересное предупреждение анализатора PVS-Studio, которому я решил посвятить отдельную заметку. Вот оно:

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

В деструкторах можно вызывать виртуальные функции, и стандарт C++ чётко описывает, как такой вызов работает. К сожалению, такой код прямо-таки притягивает ошибки, поэтому многие стандарты кодирования и анализаторы рекомендуют не делать таких вызовов. В своё время я написал на эту тему статью "Вызов виртуальных функций в конструкторах и деструкторах (C++)". Если вы новичок в C++ или подзабыли, как всё это работает, то предлагаю перед продолжением чтения заглянуть в неё. Также предлагаю прочитать её тем, кто не понимает, о каких возможных ошибках идёт речь.

Код, относящийся к предупреждению, достаточно большой, но вы смело можете его пропустить. Ниже мы разберём всё на синтетических примерах.

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;
}

Разбор ошибки на синтетическом коде

Теперь давайте разберёмся, в чём суть проблемы, используя синтетический код и сайт Compiler Explorer, чтобы быстро изучать работу этого кода.

Нужно создать иерархию классов, управляющую некими ресурсами. При этом, используя полиморфизм, мы будем работать с объектами через указатель на базовый класс.

Начнём с простейшего базового класса:

#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;
}

Пока всё хорошо. Запускаемый online пример распечатывает:

~A()

Далее выясняется, что время от времени нужно сбрасывать состояние класса, то есть освобождать ресурсы, не дожидаясь вызова деструктора при разрушении класса. В этот момент допускается ошибка проектирования с созданием виртуальной функции для очистки класса. Разработчик создаёт вот такой дополнительный виртуальный интерфейс класса:

#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;
}

Online пример печатает:

~A()
A::Reset()

Добавилась виртуальная функция Reset, освобождающая ресурсы. Чтобы не дублировать код, деструктор теперь не сам освобождает ресурсы, а просто вызывает эту функцию.

Пока кажется, что всё по-прежнему хорошо, но давайте добавим класс-наследник:

#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;
}

Online пример печатает:

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

Если из внешнего кода явно вызываем функцию Reset, то всё отрабатывает хорошо. Вызывается B::Reset(), которая затем вызывает одноимённую функцию из базового класса.

Проблема с деструктором. Каждый деструктор вызывает функцию Reset. Возникает избыточность, так как функция Reset вызывает свой вариант из базового класса.

Если мы продолжим наследоваться дальше, то всё больше будем усугублять проблему. Всё больше и больше лишних вызовов функций.

Вывод кода, где добавлен ещё один класс:

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

Ошибка проектирования класса очевидна. Впрочем, это сейчас она очевидна нам. В реальном проекте такие ошибки вполне себе могут жить, оставаясь незамеченными: код ведь работает, и функции сброса состояния класса справляются со своей задачей. Проблема в избыточных действиях и неэффективности.

Заметив описанную ошибку и пытаясь её исправить, программист рискует допустить две другие типовые ошибки.

Первый вариант. Объявить функции Reset невиртуальными и не вызывать в них базовые варианты (x::Reset). Тогда каждый деструктор будет вызывать только функцию Reset из своего класса и освобождать только свои ресурсы. Это действительно уберёт избыточность при работе деструкторов. Однако сломается очистка состояния объекта при вызове Reset извне. Сломанный код распечатает:

A::Reset()   // Сломали очистку ресурсов из вне
------------
~C()
C::Reset()
~B()
B::Reset()
~A()
A::Reset()

Второй вариант. Вызвать виртуальную функцию Reset однократно из деструктора базового класса. Это не будет работать, так как согласно правилам C++ будет вызвана функция Reset, реализованная в базовом классе, а не в наследниках. Это логично, так как к моменту вызова деструктора ~A() все наследники разрушены, и вызывать функции из них нельзя. Сломанный код распечатает:

C::Reset()
B::Reset()
A::Reset()
------------
~C()
~B()
~A()
A::Reset()  // Освобождаем ресурсы только в базовом классе

Именно этот тип ошибки и был найден в проекте qdEngine благодаря PVS-Studio. При желании теперь можете вернуться к началу статьи и посмотреть соответствующий код из движка.

Исправленный синтетический код

Как же правильно реализовать классы, чтобы избежать множественных избыточных вызовов?

Нужно отделить очистку ресурсов в классах от интерфейса для их очистки извне. Следует создать невиртуальные функции, отвечающие только за очистку данных в классах, где они объявлены. Назовём их ResetImpl и сделаем приватными, так как они не предназначены для внешнего использования.

Деструкторы просто будут делегировать свою работу функциям ResetImpl.

Функция Reset останется публичной и виртуальной. Она будет очищать данные всех классов, используя всё те же вспомогательные функции ResetImpl.

Соберём всё вместе и напишем корректный код:

#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;
}

Online пример печатает:

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

Не могу сказать, что синтетический код выглядит красиво. В нём рябит от букв A, B, C, и очень легко опечататься. Простим это синтетическим примерам. Главное, что код работает, и мы избавились от избыточных операций.

Примечание

Схожая проблема может происходить с конструкторами, когда используют функции инициализации, которые вызываются в базовых классах по несколько раз. Пример такой ситуации и её исправления представлен в видео про code-review проекта Lyra (раздел "Работаем с Replit"). Смотреть, начиная с 0:35:01.

Заключение

Сам по себе вызов виртуальной функции в деструкторе не обязательно является ошибкой. Однако это может являться признаком плохого дизайна классов. Как раз это мы и наблюдали в случае проекта qdEngine.

Анализатор PVS-Studio выдаёт предупреждение V1053 в том случае, если виртуальная функция вызывается в конструкторе или деструкторе. Это повод ещё раз проверить такой код, и, возможно, исправить его или провести рефакторинг.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. C++: freeing resources in destructors using helper functions.