В этой статье мы рассмотрим, как правильно разрушать объекты в ООП программе на языке 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.
Комментарии (4)
Racheengel
23.05.2024 12:45+2Вызов виртуальных фунцкций в деструкторе - это практически абсолютное зло. Единственное, когда такой вызов может быть легитимен - это вызов из деструктора самого крайнего класса, когда ещё не разрушены остальные. Но за этим надо следить, правильно представлять себе цепочку освобождения ресурсов.
tolich_the_shadow
23.05.2024 12:45+1Если необходимо вызвать виртуальную функцию из деструктора, имеет смысл указать квалификатор класса, чтобы намерение невиртуального вызова было очевидно.
Zara6502
на картинке оммаж на братьев-пилотов и слона?
Andrey2008 Автор
Да )