В этой статье мы рассмотрим, как правильно разрушать объекты в ООП программе на языке C++, не выполняя избыточных операций. Этим мы завершим цикл публикаций, посвящённый обзору ошибок в игровом движке qdEngine.
Неудачная реализация очистки ресурсов в коде qdEngine
Предыдущие статьи о проверке игрового движка qdEngine:
После этих публикаций у меня осталось ещё одно интересное предупреждение анализатора 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++ или подзабыли, как всё это работает, то предлагаю перед продолжением чтения заглянуть в неё. Также предлагаю прочитать её тем, кто не понимает, о каких возможных ошибках идёт речь.
Код, относящийся к предупреждению, достаточно большой, но вы смело можете его пропустить. Ниже мы разберём всё на синтетических примерах.
В деструкторе базового класса вызывается функция Finit. Поскольку в этот момент класс-наследник DDraw_grDispatcher уже разрушен, то его функция Finit не будет вызвана.
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; }
~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; }
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; }
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.
