В этой статье мы рассмотрим, как статический анализатор PVS-Studio воодушевляет заняться рефакторингом кода. Ведь чем короче, проще и понятнее код, тем меньше в нём ошибок.
В предыдущей статье "Проверка игрового движка qdEngine, часть первая: топ 10 предупреждений PVS-Studio" мы уже рассматривали интересный пример рефакторинга кода, к которому подтолкнуло предупреждение PVS-Studio (см. десятый фрагмент кода). Давайте рассмотрим на примерах, как ещё можно улучшить код в процессе использования статического анализатора.
class qdCamera
{
....
sGridCell *Grid;
....
};
bool qdCamera::restore(sGridCell *grid,
int sx, int sy,
int csx, int csy)
{
if (Grid)
delete[] Grid;
Grid = new sGridCell[sx * sy];
if (!Grid)
return false;
memcpy(Grid, grid, sizeof(sGridCell) * sx * sy);
GSX = sx;
GSY = sy;
cellSX = csx;
cellSY = csy;
return true;
}Перед нами функция, перевыделяющая буфер нужного размера. Анализатор обращает внимание на этот код, поскольку видит бессмысленную проверку указателя, который возвращает оператор new[]. Речь идёт об этих строках:
Grid = new sGridCell[sx * sy];
if (!Grid)
return false;Предупреждение PVS-Studio: V668 There is no sense in testing the 'Grid' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. qd_camera.cpp 910
Действительно, если недостаточно памяти, чтобы создать массив, оператор new[] сгенерирует исключение типа std::bad_alloc. Нулевой указатель можно получить только используя new (std::nothrow) [], но это не наш случай.
Функция, из которой удалили лишнюю проверку:
bool qdCamera::restore(sGridCell *grid,
int sx, int sy,
int csx, int csy)
{
if (Grid)
delete[] Grid;
Grid = new sGridCell[sx * sy];
memcpy(Grid, grid, sizeof(sGridCell) * sx * sy);
GSX = sx;
GSY = sy;
cellSX = csx;
cellSY = csy;
return true;
}Теперь функция restore всегда возвращает только true. Она или кидает исключение, или корректно отрабатывает. Больше нет смысла возвращать статус успешной работы (true или false). Можно упростить функцию, убрав возвращаемое значение:
void qdCamera::restore(sGridCell *grid,
int sx, int sy,
int csx, int csy)
{
if (Grid)
delete[] Grid;
Grid = new sGridCell[sx * sy];
memcpy(Grid, grid, sizeof(sGridCell) * sx * sy);
GSX = sx;
GSY = sy;
cellSX = csx;
cellSY = csy;
}Дополнительный бонус: в других местах больше не нужно проверять, что вернула эта функция.
Что ещё можно улучшить? Нет смысла проверять указатель перед вызовом оператора delete[]. Оператор корректно обрабатывает нулевые указатели (ничего не делает в этом случае).
Кто-то может возразить, что такая проверка — это микрооптимизация, так как если указатель нулевой, то не надо вызывать оператор. Это глупость. Временем вызова функции можно пренебречь на фоне таких тяжеловесных операций, как освобождение и выделение памяти. Я уже рассуждал на эту тему в статье "Следует ли проверять указатель на NULL перед вызовом функции free?". Как следует из названия, там про функцию free, но с оператором delete всё то же самое.
Код без лишних проверок:
void qdCamera::restore(sGridCell* grid,
int sx, int sy,
int csx, int csy)
{
delete[] Grid;
Grid = new sGridCell[sx*sy];
memcpy(Grid, grid, sizeof(sGridCell) * sx * sy);
GSX = sx;
GSY = sy;
cellSX = csx;
cellSY = csy;
}Мы существенно сократили код, ничего не потеряв. К сожалению, и старый, и новый код по-прежнему имеют слабое место:
delete[] Grid;
Grid = new sGridCell[sx * sy];
....
qdCamera::~qdCamera()
{
if (GSX)
{
delete[] Grid;
}
}Если в момент вызова оператора new возникнет исключение, то начнётся раскрутка стека и уничтожение различных объектов. В том числе вызовется и деструктор самого объекта qdCamera. При этом указатель Grid может быть не нулевым, но уже и не валидным. Массив был уничтожен как раз перед вызовом оператора new. Результат: неопределённое поведение.
Самая простая правка — добавить обнуление указателя:
delete[] Grid;
Grid = nullptr;
Grid = new sGridCell[sx * sy];Это плохая правка. Наша энергия уходит не туда. Главная проблема этого кода — использование ручного управления памятью. Оно провоцирует написание длинного кода, в котором легко ошибиться.
Правильное решение — использовать умный указатель или std::vector. Давайте начнём с умного указателя std::unique_ptr, так как std::vector удобен и эффективен не во всех сценариях.
class qdCamera
{
....
std::unique_ptr<sGridCell[]> Grid;
~qdCamera() = default;
....
};
void qdCamera::restore(sGridCell *grid,
int sx, int sy,
int csx, int csy)
{
Grid = std::make_unique<sGridCell[]>(sx * sy);
std::copy(grid, grid + sx * sy, Grid.get());
GSX = sx;
GSY = sy;
cellSX = csx;
cellSY = csy;
}Красота:
Напоследок вариант с использование std::vector.
class qdCamera
{
....
std::vector<sGridCell> Grid;
~qdCamera() = default;
....
};
void qdCamera::restore(sGridCell *grid,
int sx, int sy,
int csx, int csy)
{
Grid.clear();
Grid.reserve(sx * sy);
std::copy(grid, grid + sx * sy, std::back_inserter(Grid));
GSX = sx;
GSY = sy;
cellSX = csx;
cellSY = csy;
}При этом, если в процессе рефакторинга переходить на контейнеры, то изменится и интерфейс многих функций. С большой вероятностью функция restore станет такой:
void qdCamera::restore(const std::vector<sGridCell> &grid,
int sx, int sy,
int csx, int csy)
{
Grid = grid;
GSX = sx;
GSY = sy;
cellSX = csx;
cellSY = csy;
}Впрочем, я сильно удаляюсь от сути повествования. Понятно, что в процессе обширного рефакторинга код может стать вообще другим, а функция restore, например, исчезнуть или трансформироваться во что-то иное.
Главная мысль: предупреждения анализатора PVS-Studio помогают писать короткий безопасный код.
В классе arButton ручное управление памятью выражено ещё более ярко, чем в предыдущем случае. Класс создаёт в себе копии различных строк и может возвращать указатель на них.
Функции создания копий строк:
class arButton
{
....
void set_obj(const char* p)
{ if(obj_name) free(obj_name);
obj_name = strdup(p); }
void set_obj_regvalue(const char* p)
{ if(obj_name_regvalue) free(obj_name_regvalue);
obj_name_regvalue = strdup(p); }
void set_cmdline(const char* p)
{ if(cmd_line) free(cmd_line); cmd_line = strdup(p); }
void set_regkey(const char* p)
{ if(reg_key) free(reg_key); reg_key = strdup(p); }
void set_reg_exec_path(const char* p)
{ if(reg_exec_path_value) free(reg_exec_path_value);
reg_exec_path_value = strdup(p); }
void set_checkstr(const char* p)
{ if(check_after_exec) free(check_after_exec);
check_after_exec = strdup(p); }
....
char *obj_name;
char *obj_name_regvalue;
char *cmd_line;
char *reg_key;
char *reg_exec_path_value;
char *check_after_exec;
};Соответственно, для каждой строки есть свой "get", но они нам пока не интересны. Для примера приведу только один:
char* get_obj(void) { return obj_name; }Анализатор ругается на этот класс шестью однотипными предупреждениями про потенциальные утечки памяти. Приведу только одно предупреждение PVS-Studio, чтобы не загромождать статью: V773 The 'obj_name' pointer was not released in destructor. A memory leak is possible. ar_button.cpp 50
Всё дело в том, что деструктор класса ничего не делает.
arButton::~arButton(void)
{
}Создаваемые с помощью strdup строки в функциях set_* не уничтожаются. Возможны утечки памяти.
Можно починить код, доработав деструктор, но нет даже желания выполнять этот промежуточный шаг. Дело в том, что код хочется не только исправить, но и сразу улучшить. Давайте ещё раз посмотрим на одну из set-функций:
void set_obj(const char* p)
{ if(obj_name) free(obj_name);
obj_name = strdup(p); }Слабости кода:
Можно переписать код, используя умные указатели. Но это бессмысленно, так как идеальным вариантом является использование std::string. В конце концов, не зря же программа на C++, а не на C.
class arButton
{
....
void set_obj(const char* p) { obj_name = p; }
void set_obj_regvalue(const char* p) { obj_name_regvalue = p; }
void set_cmdline(const char* p) { cmd_line = p; }
void set_regkey(const char* p) { reg_key = p; }
void set_reg_exec_path(const char* p) { reg_exec_path_value = p; }
void set_checkstr(const char* p) { _after_exec = p; }
....
std::string obj_name;
std::string obj_name_regvalue;
std::string cmd_line;
std::string reg_key;
std::string reg_exec_path_value;
std::string check_after_exec;
};Get-функции в зависимости от масштаба рефакторинга будут такими:
const char* get_obj(void) const { return obj_name.c_str(); }Или такими:
const std::string& get_obj(void) const { return obj_name; }Вернёмся к set-функциям. Они ещё не доведены до состояния "хорошо", но без внешнего контекста уже сложно говорить, как лучше их дальше дорабатывать. Пока у них есть следующие недостатки:
Абстрактный вариант возможного улучшения:
....
void set_cmdline(std::string s) noexcept { cmd_line = std::move(s); }
void set_regkey(std::string s) noexcept { reg_key = std::move(s); }
....Давайте на этом остановимся, иначе дальше встанет вопрос, а нужна ли прослойка в виде get/set-функций? Возможно, нужен ещё более всеобъемлющий рефакторинг, и получится совсем другое устройство класса.
Возможно, некоторые читатели немного озадачены и думают:
"Зачем всё это было? И так понятно, что ручное управление памятью — это плохо и провоцирует ошибки. Зачем было упоминать анализатор? Без него понятно, что показанный код следует переписать".
Согласен, но обратите внимание на два следующих момента:
Спасибо за внимание! Скачивайте PVS-Studio, чтобы делать код лучше.