Не так давно, один из сотрудников покинул наш коллектив и присоединился к компании, занимающийся разработкой программного обеспечения, связанного с встраиваемыми системами. Ничего особенного в этом нет, всегда и везде, кто-то уходит, а кто-то приходит. Всё зависит от количества плюшек, удобства и предпочтений. Интересно другое. Человек искренне переживает за состояние кода на новом месте работы, что в результате и вылилось в эту совместную статью. Тяжело, «просто программировать», когда знаешь, что такое статический анализ кода.
Заповедники
Мне кажется, в мире сложилась интересная ситуация. Что происходит, если отдел программирования, это всего лишь небольшой вспомогательный элемент, не имеющий к основной сфере деятельности организации прямого отношения? Возникает заповедник. Сфера деятельности организации может быть сколь угодно важной и ответственной (медицина, военная техника). Всё равно образуется болотце, где завязают новые идеи и используются технологии 10-летней давности.
Приведу пару штрихов из переписки с одним человеком, работающим в отделе программирования на АЭС:
А он мне отвечает: Зачем нам git? Вот смотри, у меня всё в тетрадке записано.
…
А у вас вообще есть какой-то контроль версий?
2 человека используют git. Остальная контора в лучшем случае нумерованные zip’ы. Хотя насчет зипов, это я только про 1 человека уверен.
Прошу сильно не пугаться. ПО, разрабатываемое на АЭС разное бывает, плюс никто аппаратную защиту не отменяет. В этом отделе занимаются сбором и обработкой статистических данных. Но всё равно, тенденция заболачивания четко прослеживается. Я не знаю, отчего так происходит, но это так. При чем, чем больше компания, тем сильнее проявляется этот эффект.
Хочу подчеркнуть, что застой в больших организациях явление международное. У иностранцев дела обстоят в точности также. Долго искал, но так и не смог найти одну очень подходящую статью. Название тоже не помню. Если кто-то подскажет, добавлю ссылку. В ней программист рассказывает историю, как работал в одном военном ведомстве. Ведомство было естественно жутко секретное и жутко бюрократическое. Настолько секретное и бюрократичное, что в течение нескольких месяцев не могли согласовать, какие права ему выделить для работы с компьютером. В результате, он писал программу в Notepad (не компилируя). А потом его уволили за неэффективность.
Лесничие
Вернемся к нашему бывшему сотруднику. Придя на новое место работы, он испытал небольшой культурный шок. Тяжело после возни с инструментами статического анализа видеть, как игнорируются даже предупреждения компилятора. Вообще, это как изолированный мир, где программируют по своим канонам и нередко используют свои собственные термины. Больше всего из его рассказов мне понравилось словосочетание «заземлённые указатели». Прослеживается близость к аппаратной части.
Мы горды, что вырастили в нашем коллективе квалифицированного специалиста, заботящегося о качестве и надежности кода. Он не принял молча сложившуюся ситуацию, а пытается исправить её.
Для начала он сделал следующее. Он прочитал предупреждения компилятора. Далее он проверил проект, используя Cppcheck. Помимо внесения исправлений, он задумался о предотвращении типовых ошибок.
Одним из первых шагов стала подготовка им документа, нацеленного на повышение качества создаваемого кода. Ещё одним шагом может стать внедрение в процесс разработки статического анализатор кода. О PVS-Studio пока речи не идёт. Во-первых, это Linux. Во вторых продать в подобные организации ПО дело непростое. Пока, выбор пал на Cppcheck. Это очень хороший инструмент для первого знакомства людей с методологией статического анализа.
Предлагаю познакомиться, с подготовленным им документом «Как не надо писать программы». Многие пункты могут показаться написанными в стиле капитана очевидности. Однако, это реальные проблемы, которые он пытается предотвратить.
Как не надо писать программы
Пункт N1
Игнорирование предупреждений компилятора. При их большом списке можно легко упустить реальные программные ошибки, появившиеся в вновь написанном коде. Поэтому следует устранять все предупреждения.
Пункт N2
В условии оператора ‘if’ происходит не проверка значения, а присваивание:
if (numb_numbc[i] = -1) { }В данном случае код компилируется, но компилятор выдает предупреждение. Корректной будет такая запись:
if (numb_numbc[i] == -1) { }
Пункт N3
Запись вида «using namespace std;» в заголовочных файлах может приводить к тому, что будет использована эта область видимости во всех файлах, включающих этот заголовочный файл. Это может привести к тому, что будут выбраны не те функции или возникнет конфликт имен.
Пункт N4
Сравнение знаковых и беззнаковых переменных:
unsigned int BufPos; std::vector<int> ba; .... if (BufPos * 2 < ba.size() - 1) { }Помните, что при смешивании знаковых и беззнаковых переменных:
- может произойти переполнение;
- может возникнуть всегда истинное или ложное условие и как следствие вечный цикл;
- в знаковой переменную может быть помещено значение более INT_MAX (и она будет иметь отрицательное значение);
- переменная типа int при сложении/вычитании/… с переменной unsigned типа, тоже становится unsigned (отрицательные значения превращаются в большие положительные числа);
- прочие неожиданности и радости
Приведённый пример некорректно обрабатывает ситуацию, когда массив ‘ba’ пуст. Выражение «ba.size() — 1» имеет беззнаковый тип size_t. Если в массиве нет элементов, результат выражения равен 0xFFFFFFFFu.
Пункт N5
Пренебрежение использованием константности может привести к невозможности заметить трудно устраняемые ошибки. Например:
void foo(std::string &str) { if (str = "1234") { } }В данном примере оператор ‘=’ перепутан с оператором ‘==’. Если бы переменная ‘str’ была объявлена как константная, то такой код даже не скомпилировался бы.
Пункт N6
Сравниваются не строки, а указатели на строки:
сhar TypeValue [4]; ... if (TypeValue == "S") {}Даже если в переменной TypeValue будет находиться строка «S» такое сравнение всегда будет возвращать ‘false’. Корректным будет использовать функции для сравнения строк ‘strcmp’ или ‘strncmp’.
Пункт N7
Выход за границы буфера:
memset(prot.ID, 0, sizeof(prot.ID) + 1);Такой код может привести к тому, что несколько байт памяти находящейся следом за ‘prot.ID’ так же будет заполнена нулями.
Не следует путать sizeof() и strlen(). Оператор sizeof() возвращает полный размер объекта в байтах. Функция strlen() возвращает длину строки в символах (без учета терминального нуля).
Пункт N8
Недостаточное заполнение буфера:
struct myStruct { float x, y, h; }; myStruct *ptr; .... memset(ptr, 0, sizeof(ptr));В данном случае нулями будет заполнена не вся структура ‘*ptr’, а только N байт (N — размер указателя в данной платформе). Корректным будет такой код:
myStruct *ptr; .... memset(ptr, 0, sizeof(*ptr));
Пункт N9
Некорректное выражение:
if (0 < L < 2 * M_PI) { }С точки зрения компилятора здесь нет ошибки, однако оно не имеет смысла, при выполнении всегда будет получено значение ‘true’ или ‘false’ в зависимости от операторов сравнения и граничных условий. Компилятор выдает предупреждение на такую запись. Корректно будет записать этот код так:
if (0 < L && L < 2 * M_PI) { }
Пункт N10
unsigned int K; .... if (K < 0) { } ... if (K == -1) { }Беззнаковые переменные не могут быть меньше нуля.
Пункт N11
Сравнение переменной со значением, которое оно не может достигнуть ни при каких условиях. Пример:
short s; ... If (s==0xaaaa) { }О таких случаях предупреждает компилятор.
Пункт N12
Выделяем памяти через ‘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]); ... }
Пункт N13
Выделение памяти через ‘new[]’, а освобождение через ‘delete’. Или наоборот выделение — ‘new’, а освобождение через ‘delete[]’. Почему это плохо, можно почитать здесь: "delete, new[] в C++ и городские легенды об их сочетании".
Пункт N14
Использование неинициализированных переменных:
int sum; ... for (int i = 0; i < 10; i++) { sum++; }В Си/Си++ переменная по умолчанию не инициализируется нулём. Иногда может казаться, что этот код работает. Это не так. Это просто везение.
Пункт N15
Возвращение из функции ссылки или указателя на локальные объекты:
char* CreateName() { char FileName[100]; ... return FileName; }После выхода из функции ‘FileName’ будет указывать на уже освобожденную память, поскольку все локальные объекты создаются на стеке, и дальнейшая корректная работа с ней будет невозможна.
Пункт N16
Не проверять возвращаемое значение из функций, которые могут вернуть код ошибки или ‘-1’ в случае ошибки. При некорректной работе может случиться так, что функция вернет код ошибки, но мы на него никак не отреагируем и продолжим работу, а затем программа завершится некорректно в совершенно непредсказуемом месте. Такие моменты можно долго отлаживать впоследствии.
Пункт N17
Пренебрежение использованием специальных инструментов статического и динамического анализов, а так же написанием и использованием Unit-тестов.
Пункт N18
Жадничаем поставить скобки в математических выражениях. В результате получаем:
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’. Результат — условие всегда истинно.
Пункт N19
Выход за границы массива:
int mas[3]; mas[0] = 1; mas[1] = 2; mas[2] = 3; mas[3] = 4;Выражение ‘mas[3] = 4;’ обращается к несуществующему элементу массива, поскольку при объявлении массива ‘int mas[N]’ индексация его элементов возможна в интервале [0…N-1].
Пункт N20
Перепутаны приоритеты логических операций ‘&&’ и ‘||’. Оператор ‘&&’ имеет более высокий приоритет, поэтому в таком условии:
if (A || B && C) { }Сначала выполнится ‘B &&C’, а затем оставшаяся часть выражения. Это может не соответствовать требуемой логике работы программы. Часто предполагается, что логические выражения выполняются слева-направо. О таких подозрительных местах так же предупреждает компилятор.
Пункт N21
Результат присваивания не будет иметь эффекта за пределами функции:
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) {....}
Список рекомендуемой литературы:
- ВЕРЕВКА ДОСТАТОЧНОЙ ДЛИНЫ, ЧТОБЫ ВЫСТРЕЛИТЬ СЕБЕ В НОГУ. Правила программирования на С и С++. Ален И. Голуб;
- Стандарты программирования на С++. 101 правило и рекомендация. Герб Саттер, Андрей Александреску;
- Совершенный код. С. Макконнелл;
- Скользкие места Си++. Стефан К. Дьюхэрст;
- Эффективное использование C++. 50 рекомендаций по улучшению ваших программ и проектов. Скот Майерс.
Заключение
Особенных выводов у меня нет. Знаю только, что где-то в одном конкретном месте, теперь ситуация с разработкой ПО станет лучше. Это приятно.
Немного портит настроение, что многие люди даже не слышали про статический анализ. Причем, часто эти люди занимаются серьезными и ответственными вещами. Сфера программирования развивается очень активно. В результате тем, кто постоянно «работает работу», не удается следить за тенденциями и инструментарием. Они начинают работать намного менее эффективно, чем программисты, занятые во фрилансе, в стартапах, небольших компаниях.
Вот и получается странная картина. Молодой фрилансер может выполнять работу более качественно (благодаря знаниям: TDD, непрерывная интеграция, статический анализ, система контроля версий, …), чем программист 10 лет проработавший на РЖД/АЭС/(подставить что-то крупное). Слава богу, это далеко не всегда так. Но всё-таки что-то такое есть.
Почему меня это грустит? Туда бы продавать PVS-Studio. А там даже не догадываются о существовании и пользе таких инструментов. 🙂
ссылка на оригинал статьи http://habrahabr.ru/company/pvs-studio/blog/191688/
Добавить комментарий