Не так давно, один из сотрудников покинул наш коллектив и присоединился к компании, занимающийся разработкой программного обеспечения, связанного с встраиваемыми системами. Ничего особенного в этом нет, всегда и везде, кто-то уходит, а кто-то приходит. Всё зависит от количества плюшек, удобства и предпочтений. Интересно другое. Человек искренне переживает за состояние кода на новом месте работы, что в результате и вылилось в эту совместную статью. Тяжело, "просто программировать", когда знаешь, что такое статический анализ кода.
Мне кажется, в мире сложилась интересная ситуация. Что происходит, если отдел программирования, это всего лишь небольшой вспомогательный элемент, не имеющий к основной сфере деятельности организации прямого отношения? Возникает заповедник. Сфера деятельности организации может быть сколь угодно важной и ответственной (медицина, военная техника). Всё равно образуется болотце, где завязают новые идеи и используются технологии 10-летней давности.
Приведу пару штрихов из переписки с одним человеком, работающим в отделе программирования на АЭС:
А он мне отвечает: Зачем нам git? Вот смотри, у меня всё в тетрадке записано.
...
А у вас вообще есть какой-то контроль версий?
2 человека используют git. Остальная контора в лучшем случае нумерованные zip'ы. Хотя насчет зипов, это я только про 1 человека уверен.
Прошу сильно не пугаться. ПО, разрабатываемое на АЭС разное бывает, плюс никто аппаратную защиту не отменяет. В этом отделе занимаются сбором и обработкой статистических данных. Но всё равно, тенденция заболачивания четко прослеживается. Я не знаю, отчего так происходит, но это так. Причем, чем больше компания, тем сильнее проявляется этот эффект.
Хочу подчеркнуть, что застой в больших организациях явление международное. У иностранцев дела обстоят в точности также. Долго искал, но так и не смог найти одну очень подходящую статью. Название тоже не помню. Если кто-то подскажет, добавлю ссылку. В ней программист рассказывает историю, как работал в одном военном ведомстве. Ведомство было естественно жутко секретное и жутко бюрократическое. Настолько секретное и бюрократичное, что в течение нескольких месяцев не могли согласовать, какие права ему выделить для работы с компьютером. В результате, он писал программу в Notepad (не компилируя). А потом его уволили за неэффективность.
Вернемся к нашему бывшему сотруднику. Придя на новое место работы, он испытал небольшой культурный шок. Тяжело после возни с инструментами статического анализа видеть, как игнорируются даже предупреждения компилятора. Вообще, это как изолированный мир, где программируют по своим канонам и нередко используют свои собственные термины. Больше всего из его рассказов мне понравилось словосочетание "заземлённые указатели". Прослеживается близость к аппаратной части.
Мы горды, что вырастили в нашем коллективе квалифицированного специалиста, заботящегося о качестве и надежности кода. Он не принял молча сложившуюся ситуацию, а пытается исправить её.
Для начала он сделал следующее. Он прочитал предупреждения компилятора. Далее он проверил проект, используя Cppcheck. Помимо внесения исправлений, он задумался о предотвращении типовых ошибок.
Одним из первых шагов стала подготовка им документа, нацеленного на повышение качества создаваемого кода. Ещё одним шагом может стать внедрение в процесс разработки статического анализатор кода. О PVS-Studio пока речи не идёт. Во-первых, это Linux. Во вторых продать в подобные организации ПО дело непростое. Пока, выбор пал на Cppcheck. Это очень хороший инструмент для первого знакомства людей с методологией статического анализа.
Предлагаю познакомиться, с подготовленным им документом "Как не надо писать программы". Многие пункты могут показаться написанными в стиле капитана очевидности. Однако, это реальные проблемы, которые он пытается предотвратить.
Игнорирование предупреждений компилятора. При их большом списке можно легко упустить реальные программные ошибки, появившиеся в вновь написанном коде. Поэтому следует устранять все предупреждения.
В условии оператора 'if' происходит не проверка значения, а присваивание:
if (numb_numbc[i] = -1) { }В данном случае код компилируется, но компилятор выдает предупреждение. Корректной будет такая запись:
if (numb_numbc[i] == -1) { }Запись вида "using namespace std;" в заголовочных файлах может приводить к тому, что будет использована эта область видимости во всех файлах, включающих этот заголовочный файл. Это может привести к тому, что будут выбраны не те функции или возникнет конфликт имен.
Сравнение знаковых и беззнаковых переменных:
unsigned int BufPos;
std::vector<int> ba;
....
if (BufPos * 2 < ba.size() - 1) { }Помните, что при смешивании знаковых и беззнаковых переменных:
Приведённый пример некорректно обрабатывает ситуацию, когда массив 'ba' пуст. Выражение "ba.size() - 1" имеет беззнаковый тип size_t. Если в массиве нет элементов, результат выражения равен 0xFFFFFFFFu.
Пренебрежение использованием константности может привести к невозможности заметить трудно устраняемые ошибки. Например:
void foo(std::string &str)
{
if (str = "1234")
{
}
}В данном примере оператор '=' перепутан с оператором '=='. Если бы переменная 'str' была объявлена как константная, то такой код даже не скомпилировался бы.
Сравниваются не строки, а указатели на строки:
char TypeValue [4];
...
if (TypeValue == "S") {}Даже если в переменной TypeValue будет находиться строка "S" такое сравнение всегда будет возвращать 'false'. Корректным будет использовать функции для сравнения строк 'strcmp' или 'strncmp'.
Выход за границы буфера:
memset(prot.ID, 0, sizeof(prot.ID) + 1);Такой код может привести к тому, что несколько байт памяти находящейся следом за 'prot.ID' так же будет заполнена нулями.
Не следует путать sizeof() и strlen(). Оператор sizeof() возвращает полный размер объекта в байтах. Функция strlen() возвращает длину строки в символах (без учета терминального нуля).
Недостаточное заполнение буфера:
struct myStruct
{
float x, y, h;
};
myStruct *ptr;
....
memset(ptr, 0, sizeof(ptr));В данном случае нулями будет заполнена не вся структура '*ptr', а только N байт (N - размер указателя в данной платформе). Корректным будет такой код:
myStruct *ptr;
....
memset(ptr, 0, sizeof(*ptr));Некорректное выражение:
if (0 < L < 2 * M_PI) { }С точки зрения компилятора здесь нет ошибки, однако оно не имеет смысла, при выполнении всегда будет получено значение 'true' или 'false' в зависимости от операторов сравнения и граничных условий. Компилятор выдает предупреждение на такую запись. Корректно будет записать этот код так:
if (0 < L && L < 2 * M_PI) { }unsigned int K;
....
if (K < 0) { }
...
if (K == -1) { }Беззнаковые переменные не могут быть меньше нуля.
Сравнение переменной со значением, которое оно не может достигнуть ни при каких условиях. Пример:
short s;
...
If (s==0xaaaa) { }О таких случаях предупреждает компилятор.
Выделяем памяти через 'new' или 'malloc' и забываем ее освободить через 'delete'/'free' соответственно. Например, может быть такой код:
void foo()
{
std::vector<int> *v1 = new std::vector<int>;
std::vector<int> v2;
v2->push_back(*v1);
...
}Скорее всего, раньше в 'v2' сохранялся указатель на 'std::vector<int>'. Теперь из-за изменения части кода, это не нужно и сохраняются просто значения типа 'int'. При этом мы не освобождаем память, которую выделили под 'v1', поскольку раньше это было не нужно. Для того, что бы сделать код корректным, следует добавить выражение 'delete v1' в конец функции. Или использовать умные указатели.
А ещё лучше довести рефакторинг до конца и сделать 'v1' локальным объектом, раз его больше не надо никуда передавать:
void foo()
{
std::vector<int> v1;
std::vector<int> v2;
v2->push_back(v1[0]);
...
}Выделение памяти через 'new[]', а освобождение через 'delete'. Или наоборот выделение - 'new', а освобождение через 'delete[]'. Почему это плохо, можно почитать здесь: "delete, new[] в C++ и городские легенды об их сочетании".
Использование неинициализированных переменных:
int sum;
...
for (int i = 0; i < 10; i++)
{
sum++;
}В Си/Си++ переменная по умолчанию не инициализируется нулём. Иногда может казаться, что этот код работает. Это не так. Это просто везение.
Возвращение из функции ссылки или указателя на локальные объекты:
char* CreateName()
{
char FileName[100];
...
return FileName;
}После выхода из функции 'FileName' будет указывать на уже освобожденную память, поскольку все локальные объекты создаются на стеке, и дальнейшая корректная работа с ней будет невозможна.
Не проверять возвращаемое значение из функций, которые могут вернуть код ошибки или '-1' в случае ошибки. При некорректной работе может случиться так, что функция вернет код ошибки, но мы на него никак не отреагируем и продолжим работу, а затем программа завершится некорректно в совершенно непредсказуемом месте. Такие моменты можно долго отлаживать впоследствии.
Пренебрежение использованием специальных инструментов статического и динамического анализов, а так же написанием и использованием Unit-тестов.
Жадничаем поставить скобки в математических выражениях. В результате получаем:
D = ns_vsk.bit.D_PN_ml + (int)(ns_vsk.bit.D_PN_st) << 16;В данном случае первым будет выполнено сложение, а только затем сдвиг влево. Смотри "Приоритет операций в языке Си/Си++ ". Исходя из логики программы, последовательность операций должна быть противоположной - сначала сдвиг, а зачем сложение. Похожая ошибка возникает и в таком коде:
#define A 1
#define B 2
#define TYPE A | B
if (type & TYPE) { }Здесь ошибка заключается в том, что макрос TYPE забыли окружить круглыми скобками. Поэтому сначала будет выполнено выражение 'type & A', а уже затем зачем '(type & A ) | B'. Результат - условие всегда истинно.
Выход за границы массива:
int mas[3];
mas[0] = 1;
mas[1] = 2;
mas[2] = 3;
mas[3] = 4;Выражение 'mas[3] = 4;' обращается к несуществующему элементу массива, поскольку при объявлении массива 'int mas[N]' индексация его элементов возможна в интервале [0...N-1].
Перепутаны приоритеты логических операций '&&' и '||'. Оператор '&&' имеет более высокий приоритет. Пример плохого кода:
if (A || B && C) { }Это может не соответствовать требуемой логике работы программы. Часто предполагается, что логические выражения выполняются слева-направо. О таких подозрительных местах так же предупреждает компилятор.
Результат присваивания не будет иметь эффекта за пределами функции:
void foo(int *a, int b)
{
If (b == 10)
{
*a = 10;
}
else
{
a = new int;
}
}Указателю 'a' не может быть присвоено другое значение адреса, для этого следовало бы записать объявление функции в таком виде:
void foo(int *&a, int b) {....}или:
void foo(int **a, int b) {....}Особенных выводов у меня нет. Знаю только, что где-то в одном конкретном месте, теперь ситуация с разработкой ПО станет лучше. Это приятно.
Немного портит настроение, что многие люди даже не слышали про статический анализ. Причем, часто эти люди занимаются серьезными и ответственными вещами. Сфера программирования развивается очень активно. В результате тем, кто постоянно "работает работу", не удается следить за тенденциями и инструментарием. Они начинают работать намного менее эффективно, чем программисты, занятые во фрилансе, в стартапах, небольших компаниях.
Вот и получается странная картина. Молодой фрилансер может выполнять работу более качественно (благодаря знаниям: TDD, непрерывная интеграция, статический анализ, система контроля версий, ...), чем программист 10 лет проработавший на РЖД/АЭС/(подставить что-то крупное). Слава богу, это далеко не всегда так. Но всё-таки что-то такое есть.
Почему меня это огорчает? Туда бы продавать PVS-Studio. А там даже не догадываются о существовании и пользе таких инструментов. :)