PVS-Studio corporate blog
Programming
C++
Game development
C#
Comments 26
+6
Проблема проста — не используют возвращаемое логическое значение метода empty, указывающее, пуст ли контейнер. Судя по тому, что в приведённом выражении кроме вызова метода ничего нет, можно предположить, что разработчик хотел очистить контейнер, но ошибся, вызвав метод empty вместо clear.


Проблемы не возникло бы, если бы метод назывался is_empty.
0
Автор оригинальной статьи про проверку X-Ray Engine тоже обратил на это внимание :)
0
Особенно «весело» при использовании MFC и STL в одном проекте. У той же CString::Empty() — очистка строки.
-4
и уж наверняка бы её не возникло, называйся метод get_container_size_is_equal_to_zero(); (сарказм)
0
10. Как заметили выше, изначальная ошибка не в вызове неправильного метода, а в неправильном именовании метода. Название должно давать четкое понимание, что метод делает.
9. Copy-Paste — зло! Используйте циклы с автоопределением размера массивов. И статический ассерт с проверкой что бы размеры массивов были одинаковы.
8. При создании функции логгирования нужно создать вариант с переменным листом аргументов и формирование строки засунуть внутрь WriteLine() — это проще чем при каждом вызове ещё и string.Format() звать(не знаю на счёт C#, а вот в C++ так делал).
7, 6, 5. Все мы люди, все мы ошибаемся — с опечатками ничего не поделаешь.
4. Правила MISRA указывают, что в функции должен быть один вход и один выход. Не стоит раскидывать return'ы по всем внутренностям функции. Кстати, те же правила указывают, что цепочка if-else if обязательно должна заканчиваться else, даже если в нем будет пусто.
3, 2, 1. Опять опечатки.
0
Ага, особенно последняя.
Но примерно половину ошибок можно было бы избежать, если бы использовалась правильная методология написания кода.
0
Мы много общаемся с руководителями разработок разных компаний на тему контроля качества кода. Так вот, в общем случае, кодревью служит только для проверки логики работы внесённых изменений в код, немножко code style и практически ничего (из серии «случайно бросилось в глаза») по ляпам разного характера.
0
Вообще предупреждение на первое место хорошо бы изменить: у разработчика возникнет ощущение, что ваш анализатор просто ошибся. Без вашего детального разъяснения понять где ошибка реально тяжело.
+1
А анализатор умный что ли? Он "видит" примерно то, что показал автор когда foo() использовал и применяет правила. Не удивлюсь, если авторы анализатора сами подумали что он ошибся и полезли с отладчиком спотреть почему.
0

Эх, чем больше смотрю на эти перечни "тупых" или "непреднамеренных" ошибок типа копипаста и т.п., тем больше задаюсь вопросом, сколько же тогда ошибок кроется в рантайме (когда код по задумке разработчика написан верно, так, как он и хотел, но сама логика программы при этом не верна)
Если программа работает, это не значит, что в ней нет ошибок.
Если в программе нет ошибок, это не значит, что она работает правильно.

0
И хотя я понимаю, что авторы этой статьи не смогут реализовать это, но помечтать вслух не запрещено ведь?
Круто было бы, если бы возле каждой проверки был описан результат-баг в реальном продукте. К примеру, вызвал empty вместо clear — массив не очистился, потом метод Х использовал этот массив, вернул неправильные данные методу В и тот сохранил пустой файл вместо файла с данными.
+1
Какой только фигни в коде не напишешь когда руководитель департамента теребит каждые полчаса «где релиз, когда релиз» а программистов по пальцам одной руки пересчитать можно
0
Каждый раз, как читаю о PVS возникает желание вернуться на C++. Автоматический поиск багов вызывает чувство уюта.: )
Нет ли в планах JS? Неиспользуемые параметры метода или ошибки в логических условиях выглядят реализуемо для динамического языка.
+1
За такое руки нужно отбивать! Это я не про баг, это я про его решение. Не должно быть static внутри функции, все переменные которые static должны быть объявлены в приватной части класса и документированы.
Один раз столкнулся со странным поведением — вызываю функцию класса, а она не делает того, что нужно. А оказалось у нее внутри статическая переменная в которую она сохраняет переданный параметр и на следующих входах проверяет новый параметр с предыдущим и от этого меняется поведение. И нигде такое поведение не было документировано. И вот такое статическим анализатором уже не найдешь.
0
По всей видимости нет. Но у нас при написании статей и нет задачи найти как можно больше багов в проекте. :)
0
Вопрос был еще и в том, может ли PVS-Studio находить подобную ошибку?
Суть бага:

void buggy_function()
{
float* table = nullptr;
if (some_condition) {
float another_table[] = { 0.1, 0.2, 0,3 };
table = another_table;
}
some_other_function(table);
}

Память выделенная на стеке продолжает использоваться за пределами блока через указатель, что раньше прокатывало, но при переходе на новую версию компилятора привело к трудноуловимому багу.
+1
Пфф… В таком примере и PVS-Studio её находит:

V506 CWE-562 Pointer to local variable 'x' is stored outside the scope of this variable. Such a pointer will become invalid. consoleapplication2017.cpp 31
Only those users with full accounts are able to leave comments. , please.