Статические анализаторы помогают не только обнаруживать ошибки и дефекты безопасности, но и делать код чище. Выявляя лишние проверки, дублирующие действия и другие аномалии, можно сделать код проще, красивее и легче для чтения. Разберём это на практическом примере рефакторинга функции.
Сразу перейдём к делу и начнём разбирать фрагмент кода на языке C, взятый из проекта iSulad.
/* container state to disk */
int container_state_to_disk(const container_t *cont)
{
int ret = 0;
if (cont == NULL) {
return -1;
}
ret = container_save_container_state_config(cont);
if (ret != 0) {
return ret;
}
return ret;
}На первый взгляд вполне опрятный код. Беда в том, что он избыточен и заставляет тратить на своё изучение больше времени, чем заслуживает.
Я наткнулся на этот фрагмент кода благодаря предупреждению анализатора PVS-Studio:
V523 The 'then' statement is equivalent to the subsequent code fragment. container_unix.c 878
Речь идёт об этом месте:
if (ret != 0) {
return ret;
}
return ret;Действительно, вне зависимости от того, какое значение имеет переменная ret, выполняется одно и то же действие.
Это не баг. Однако предупреждение анализатора заставляет задуматься о чистоте кода и, как вы сейчас увидите, "потянув за верёвочку", можно провести хороший рефакторинг.
Для начала сократим нижнюю часть кода до одного return:
/* container state to disk */
int container_state_to_disk(const container_t *cont)
{
int ret = 0;
if (cont == NULL) {
return -1;
}
ret = container_save_container_state_config(cont);
return ret;
}Давайте не будем останавливаться на этом и продолжим улучшать код. Объявлять переменные в начале функции — это устаревший, вредный стиль написания кода. Он берёт своё начало от языков, где в начале функции требовалось объявлять все переменные. Но в их предварительном объявлении нет ничего замечательного, а наоборот, увеличивается шанс допустить ошибку. Например, начать использовать ещё неинициализированную переменную.
Рекомендуется объявлять переменную как можно ближе к месту её использования. Более того, лучше сразу её инициализировать в точке объявления.
Здесь переменная ret тоже инициализируется при объявлении. Но это "не та инициализация", она просто не имеет смысла. В теории, она может защитить от ошибки использования неинициализированных переменных. Однако лучше писать так, чтобы в принципе такой возможности не было. Для этого как раз и рекомендуют совместить объявление переменной с её инициализацией полезным значением.
В нашем случае это выглядит так:
/* container state to disk */
int container_state_to_disk(const container_t *cont)
{
if (cont == NULL) {
return -1;
}
int ret = container_save_container_state_config(cont);
return ret;
}При этом становится очевидно, что переменная ret вообще не нужна. Это повод избавиться от неё:
/* container state to disk */
int container_state_to_disk(const container_t *cont)
{
if (cont == NULL) {
return -1;
}
return container_save_container_state_config(cont);
}Получилось упростить и сократить код. Good.
Здесь можно возразить, что предыдущий вариант кода был написан из расчёта его дальнейшего расширения. Ой, бросьте. Не надо писать код про запас. С большой вероятностью это расширение никогда не понадобится! А если такое произойдёт, то ничего сложного в добавлении кода нет.
Здесь уместна эта картинка:
Продолжим. Обратите внимание на комментарий к функции. В нём нет никакого смысла. Это калька с названия функции:
/* container state to disk */
int container_state_to_disk(const container_t *cont)Комментарий ничего не поясняет, ничем не помогает. Это бессмысленная сущность, которую следует удалить.
int container_state_to_disk(const container_t *cont)
{
if (cont == NULL) {
return -1;
}
return container_save_container_state_config(cont);
}Перед нами простая, красивая функция-прослойка, которая проверяет указатель и передаёт управление другой функции.
На этом всё? Оказывается, нет. Заглянем в ту — другую — вызываемую функцию:
static int container_save_container_state_config(const container_t *cont)
{
int ret = 0;
parser_error err = NULL;
char *json_container_state = NULL;
if (cont == NULL) {
return -1;
}
container_state_lock(cont->state);
....
}Ха! Оказывается, эта функция и сама имеет в себе проверку указателя, и ей можно безопасно передать NULL.
Соответственно код упрощается ещё больше:
int container_state_to_disk(const container_t *cont)
{
return container_save_container_state_config(cont);
}Что это значит? Что две эти функции — синонимы.
Это стоило написать в комментарии и не создавать бессмысленные проверки и действия в функции. Как мы выяснили, всё это был мусорный код, который ничего не делает и только занимает место.
Чего не хватает, так это приблизительно такого комментария:
/* На данный момент функция container_state_to_disk является
синонимом container_save_container_state_config.
Это изменится, когда будет реализовываться функционал .......
*/
int container_state_to_disk(const container_t *cont)
{
return container_save_container_state_config(cont);
}А если нет никакого "когда будет реализовываться"? Тогда текст программы можно упростить и сократить ещё больше с помощью вот такого макроса:
#define container_state_to_disk container_save_container_state_configК этому макросу даже комментарий не нужен. И так понятно, что два эти названия являются синонимами.
P.S. Вообще, я не очень люблю макросы. Но без них в C жить тяжело, и аккуратное их использование не портит и не усложняет код. Поэтому я посчитал вполне уместным всё в итоге свести к макросу.
Пишите простой код. Не пишите избыточный код про запас. Спасибо за внимание.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Static analyzer nudges you to write clean code.
Комментарии (30)
Serpentine
16.04.2024 14:43А если нет никакого "когда будет реализовываться"? Тогда текст программы можно упростить и сократить ещё больше с помощью вот такого макроса:
#define container_state_to_disk container_save_container_state_configЯ всего контекста не вижу, но, судя по всему, у функций в статье вот такие прототипы:
int container_state_to_disk(const container_t *cont); static int container_save_container_state_config(const container_t *cont);Функция container_state_to_disk() имеет внешнее связывание, а container_save_container_state_config() - статическая с внутренним связыванием, т.е. этим макросом мы сменим область видимости container_state_to_disk() со всеми вытекающими.
Andrey2008 Автор
16.04.2024 14:43Тогда можно убрать
static. Всё равно ведь есть интерфейс её дёргать из вне.
alysnix
16.04.2024 14:43какой смысл передавать гарантированно не null указатель, если это по сути ссылка. ее и надо делать параметром функции.
если параметр - указатель, это декларация того факта,что может быть подставлен nullptr. и проверка нужна по любому.
если параметр - ссылка - это декларация того, что всегда будет подставлена ссылка на существующий обьект..
Serpentine
16.04.2024 14:43+1Это сишный код, в Си нет ни ссылок, ни nullptr.
alysnix
16.04.2024 14:43ааа.. это я проглядел. тогда совет - не проверять указатель, если он формально может быть нулом - безусловно вредный.
опять же, реализация функции не должна опираться на реализацию вызываемых или вызывающих функций, а лишь соответствовать своему прототипу и строгим комментам к нему, в случае если в языке нет способов сделать более формальное описание.
ReadOnlySadUser
16.04.2024 14:43+2It depends. И очень сильно.
Обычно достаточно разумным видится обвешать все входные аргументы проверками на уровне реализаций API функций, т.е. функций принимающих недоверенные данные, а всё что "внутри" - по ситуации.Зачастую вполне можно расчитывать на инварианты, которые соблюдаются "сверху", а для того, чтобы читающий, понимал на какие инварианты опирается функция - навешать assert-ов (по сути описать контракт).
Правда на моей памяти примерно каждый второй программист ленится описывать контракты) И приходится гадать надо тут проверять на NULL или не надо (и человеку, и статическому анализатору).
alysnix
16.04.2024 14:43+3Ха! Оказывается, эта функция и сама имеет в себе проверку указателя, и ей можно безопасно передать NULL.
я лишь говорю, что вот такая сентенция автора - есть бред сумасшедшего.
он меняет вызывающую функцию, опираясь на ТЕКУЩУЮ!!! реализацию вызываемой. текущая реализация(без изменения прототипа функции) может быть изменена и это автоматически сделает невалидным код вызывающей функции. приехали. это называется - совет как писать "чистый код"
автор не учитывает, что вызываемая функция должна не только проверять указатель, но и возвратить тот же результат (-1). а это вообще нигде не оговаривается. то есть если вызываемая будет проверять указатель, но возвращать не -1 , то опять невалидность.
автор не учитывает, что есть хорошее правило - смотреть только прототипы, но не смотреть реализации.
делать надо не так. поскольку у нас чистый си, а передаем мы ссылку, то надо просто ввести дефайн
#define ref *для спецификации параметров функций, являющихся де факто ссылками, то есть не могущими быть невалидным указателем.
и писать прототипы не так -
void fun ( type *ptr )а так
void fun ( type ref ptr)и хотя это одно и то же после препроцессирования, мы таким образом можем указывать, что параметр ptr не может быть невалидным, и внутри функции не проверяется на ноль... можно дефайн и not_null назвать.
тогда в прототипе(без заглядывания в реализацию) будет четкое указание, что функция ожидает ненулевой указатель, то есть не проверяет его перед использованием.
Serpentine
16.04.2024 14:43+2делать надо не так. поскольку у нас чистый си, а передаем мы ссылку, то надо просто ввести дефайн
#define ref *На мой взгляд, вы слишком перемудрили, и это наоборот ещё больше запутает. И так видно, что передаем адрес объекта без возможности модификации значения. Кроме того, если есть непреодолимое желание, вместо него всё равно ведь можно будет передать практически любой набор байт, это же Си :)
есть хорошее правило - смотреть только прототипы, но не смотреть реализации.
Некоторые могут быть не знакомы с таким правилом, это же не догма.
Я сейчас личное мнение выскажу, но за что мне нравится Си, так это за свободу писать эффективный и аскетичный код без оглядки на авторитетное мнение (в отличие от C++ с его Core Guidelines и пр. от Страуструпа&Co, хотя и там как угодно можно писать). Руководствуемся только здравым смыслом и революционным чутьем, ну иногда стандартом и может K&R :) Понятное дело, если пишем в команде, то добавляем и другие договоренности.
Компилируемый и работающий без ошибок код на С для одного человека будет кристально чистым, а для другого может считаться неправильным.
ReadOnlySadUser
16.04.2024 14:43+1надо просто ввести дефайн
Не надо так делать :) Под "так" - я имею тащить в язык чужеродные языку абстракции. В Си нет ссылок, и не нужно их там изобретать. Это только запутывает.
В принципе, важ подход имеет место быть и более или менее правильный путь такой:
Код
#if COMPILER_IS_GNU_COMPATIBLE #define _Notnull_Args_(...) __attribute__((nonnull(__VA_ARGS__))); #define _Notnull_ #elif COMPILER_IS_MSVC #include <sal.h> #define _Notnull_Args_(...) #else #define _Notnull_Args_(...) #define _Notnull_ #endif void swap(int* a, int* b) _Notnull_Args_(1, 2); void swap(_Notnull_ int* a, _Notnull_ int* b) { int t = *a; *a = *b; *b = t; }Всё уже придумано до нас :)
Мне лично такой подход не нравится, т.к. SAL-аннотации слишком громоздкие. assert-ы проще, надёжнее, кроссплатформенны и кросскомпиляторны)
alysnix
16.04.2024 14:43чтоб увидеть ассерт, надо смотреть в реализацию. но никто не обязан вам ее давать. вот не обязан и все. и никто не обязан держать вас в курсе текущего состояния реализации. может там вообще пока пустые заглушки.
а вот декларации давать обязаны, и вся полнота(ну которая возможна) инфы должна содержаться в ней.
kekoz
16.04.2024 14:43+1тогда совет - не проверять указатель, если он формально может быть нулом - безусловно вредный.
Вопрос для уточнения позиции — могут ли указатели, передаваемые функциям из
<string.h>, формально бытьNULL?
eptr
16.04.2024 14:43Это сишный код, в Си нет ни ссылок, ни nullptr.
Ссылок пока нет, а вот
nullptrуже есть в C23, — вот пример.Serpentine
16.04.2024 14:43nullptrуже есть в C23, — вот пример.Да есть там такое, только C23 ещё не принят, его проект сейчас на стадии рассмотрения, и не все компиляторы его поддерживают, понятное дело.
Когда примут, тогда у нас будет C++ без классов :)
Totoshka25
16.04.2024 14:43Объявлять переменные в начале функции — это устаревший, вредный стиль написания кода.
...
Рекомендуется объявлять переменную как можно ближе к месту её использования.
На сколько я помню, в языке C объявление переменных должно идти в начале функции. Поэтому такая оптимизация в первом примере не имеет места быть. Но это не отменяет дальнейших упрощений
eptr
16.04.2024 14:43+1На сколько я помню, в языке C объявление переменных должно идти в начале функции.
Это — для стандарта C89/C90, начиная же с C99, это — не обязательно.
Стандарту C99 уже 25 лет, так что им уже, наверное, можно начать пользоваться.
Playa
16.04.2024 14:43Больше похоже на остатки отладочного кода. Так и вижу внутри какой-нибудь printf, который впоследствии удалили.
gun_dose
16.04.2024 14:43+1По хорошему, статический анализатор - это то, что каждый разработчик должен настроить в своей IDE первым делом. Но поскольку не все могут по-хорошему, нужно вешать проверки на пре-коммит хуки и на пайплайны в репозитории. Но самое главное - не пользоваться автоисправлением, которое умеет делать анализатор, а стараться исправлять ошибки самому, чтобы это всё откладывалось в голове. Через пару месяцев работы в таких условиях, уже сам на автомате с ходу будешь писать чистый код.
И ещё один момент, почему обязательно нужно использовать статические анализаторы: статический анализ практически полностью может заменить собой юнит-тестирование, соответственно, это возможность писать надёжный код без юнит-тестов.
voldemar_d
16.04.2024 14:43В MS Visual Studio 2019 и 2022 даже если ничего не настраивать, то встроенный статический анализатор уже есть. Не такой мощный, как PVS, но кое-что находит. Некоторые вещи прямо в синтаксисе подчёркивает, даже если явно запуск анализа не вызывать.
gun_dose
16.04.2024 14:43+1Так это есть почти в любой IDE, при условии, что она поддерживает тот язык, на котором пишешь. Но ведь есть отдельная каста разработчиков, которые считают, что IDE для дураков, а настоящие ниндзя кодят только в Vim. Но мы то знаем, что они используют Vim только потому, что не могут из него выйти :D
MiyuHogosha
16.04.2024 14:43Комментарий кпервой картинке... Не дай вам бог родиться при Генрихе VI... то есть брать госконтракты. Платить будут хотеть по ГОСТу, за строчки с корректирующимии поправками, а за несоблюдение правил отчетов о трудоемкости ПЗ может и шрафов добиться, если приспичит.
datacompboy
может, тогда стоит вообще -- массовое переименование и выбросить даже макрос?
goldexer
В общем да, но не всегда: например в эмбед - при тотальной перенастройке микроконтроллера, проще обождать пару десятков секунд новую кодогенерацию, чем руками править десятки, а то и сотни строчек чужого кода. При этом генератор стирает правки - их надо оборачивать или вносить заново сто раз на дню - уж проще макросом сослаться. Да и будущий функционал вполне себе может появиться. Кто-то с этим никогда не столкнется, а кому-то повседневная рутина.
datacompboy
А почему не использовать исходное имя?
goldexer
С радостью бы... но они ж (например ST), свой Hardware Abstraction Layer не именуют так, чтобы было понятно даже *кхм-кхм, не могу слово подобрать* чайнику, что это за функция и что за пять сокращений по две буквы в её названии за что отвечает и какие за какими их десять штук подряд надо вызывать, а потом некоторые вызывать, а некоторые нет, плюс в них такие же по стилю константы надо передавать. Получается код из шутки про очень широкий монитор и джаваскриптера. Вдобавок, сегодня они хотят коллбэки, а завтра предлагают weak функции, на, мол, сам переделывай. Но ладно бы только это, я таких три-пять строчек кода их функций (в которых так же миллион проверок, миллион уровней вложенности, блокировок и т.п.) навязанного стиля меняю на одну запись в регистр, так рядом сидит человечек, который «я тут за всё плачу, сделай мне вот чтобы вот такое название написал и оно работало, я этого ихнего не понимаю». Зажмут вот так с двух сторон и привет синонимы, обёртки, потенциальный функционал... Уж простите, что сумбурно объяснил, устал с работы
datacompboy
А, так HAL абстракции завернутые в макросы обычно идут сразу, ибо вариантов прослоек предлагается на выбор: плохо, неудобно, плохо и неудобно, или так, что вообще не разберёшь.
... и какой из них будет выбран при компиляции определяется волею рандома.
Только я не знаю кто такой iSalud упомянутый в посте -- я не думал что это нечто из embedded.
goldexer
В яблочко! Плюс болезнь мелкомягких в виде невозможности расширить некоторые окна просмотра - длинные однокоренные названия различить в полевых условиях на 15'' монике в скрин-сплите дебага и стороннего GUI...
Ну и как я уже сказал, мало кто сталкивается со специфичными условиями и жёсткой зависимостью от «менее опытных» коллег. Тестово-велосипедные костыли надо рефакторить. Желательно до того, как это разрастётся в зыбучее мховое легаси