С++: освобождение ресурсов в деструкторах с использованием вспомогательных функции

В этой статье мы рассмотрим, как правильно разрушать объекты в ООП программе на языке 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.