В этой статье мы рассмотрим, как статический анализатор PVS-Studio воодушевляет заняться рефакторингом кода. Ведь чем короче, проще и понятнее код, тем меньше в нём ошибок.
В предыдущей статье "Проверка игрового движка qdEngine, часть первая: топ 10 предупреждений PVS-Studio" мы уже рассматривали интересный пример рефакторинга кода, к которому подтолкнуло предупреждение PVS-Studio (см. десятый фрагмент кода). Давайте рассмотрим на примерах, как ещё можно улучшить код в процессе использования статического анализатора.
Переусложнённая функция restore
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::make_unique мы вообще избавились от низкоуровневых вызовов new/delete.
- Код безопасен;
- Деструктор теперь вообще не нужен.
Напоследок вариант с использование 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 помогают писать короткий безопасный код.
Управление памятью в стиле языка C в классе arButton
В классе 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); }Слабости кода:
- Проверка перед вызовом free является лишней. Функция free ничего не делает, если ей передать NULL.
- Нет проверки указателя, который возвращает функция strdup. Таким образом проверка перекладывается на внешний код. При этом вовне невозможно определить, почему функция get_* возвращает NULL. Есть два варианта. Первый: функция set_* не вызывалась. Второй: произошла ошибка выделения памяти, и работа приложения нарушена.
- Как уже было рассмотрено выше, подобный код с ручным управлением памятью провоцирует ошибки.
Можно переписать код, используя умные указатели. Но это бессмысленно, так как идеальным вариантом является использование 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-функциям. Они ещё не доведены до состояния "хорошо", но без внешнего контекста уже сложно говорить, как лучше их дальше дорабатывать. Пока у них есть следующие недостатки:
- Непонятно, как лучше обрабатывать ситуации, если в функцию будет передан nullptr. Это означает "пустая строка" или "беда, время сгенерировать исключение"?
- Копирование строк неэффективно. Прежде чем выделить буфер под строчку, придётся предварительно вычислить её длину.
Абстрактный вариант возможного улучшения:
....
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, чтобы делать код лучше.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Let's check the qdEngine game engine, part two: simplifying C++ code.
datacompboy
Должен сказать, что на фоне статей с проверками на баги, эта статья не продаёт.
Слишком много ментальных усилий ради кода, о котором думать вообще не хочется, этож бойлерплейт, про который работает -- не трожь и не думай...
Andrey2008 Автор
Не весь код можно не трогать. Статья о том, что статический анализ помогает начать рефакторинг. Анализатор даёт и повод и отправную точку для этого. А старый код он вот такой, да... У меня нет иллюзий на тему того, какой код скрывают недра реальных проектов :)