Совсем недавно на Хабре презентовали новый инструмент статического анализа С++ кода — CppCat. О предыдущем своём проекте (PVS-Studio) его авторы долго и подробно рассказывали на Хабре. Отношение к нему у меня было двоякое — с одной стороны, статический анализ, конечно, нужен. С ним лучше, чем без него. С другой стороны — PVS-Studio пугала своей масштабностью, эдакой «энтерпрайзнутостью» ну и ещё ценой. Я хорошо себе представлял, как её может купить команда проекта человек в 50, но что делать разработчику-одиночке или команде человек в 5 — я не понимал. Помнится, предлагал как-то авторам развернуть «PVS as a service» в облаке и продавать доступ к нему по времени. Но они пошли своим путём и сделали урезанную версию за относительно небольшие деньги (такой бюджет вполне реально «пробить» или даже купить просто для себя).
Давайте посмотрим, стоит ли игра свеч.
Первое, что сходу не нравится при установке — отсутствие интеграции с Visual Studio 20052008. Нет, я понимаю, что у старых версий студии был совсем другой API для расширений и может быть для его поддержки нужны дополнительные усилия, но ведь язык С++ — совсем не молодой, многие из встречаемых мною проектов до сих пор живут на VS2008 и никто не планирует их мигрировать при необходимости правок примерно десятка строк в квартал. Получается, для работы с legacy всё ещё нужна полноценная PVS-Studio. Ну ок.
Минимализм в интеграции с Visual Studio радует: менюшка на пару пунктов, один тулбар — ничего лишнего. Галка «Enable Analysis on Build» по-дефолту включена. Зачем? Мне кажется более логичным не замедлять скорость каждого билда, а сделать анализ всего проекта, к примеру, перед коммитом в репозиторий. Можно сделать себе напоминалку на пре-коммит хук svngit клиентов о том, что было бы неплохо проверить новый код статическим анализатором.
Для объективности тестов я выбрал три проекта:
- Notepad++ (который я считаю образцом плохого кода)
- ZeroMQ(который я считаю образцом хорошего кода)
- Один из моих старых рабочих проектов — писался мною много лет назад, наверняка в нём много глупых ошибок
Notepad++ и ZeroMQ были выбраны потому, что у меня был небольшой опыт в разработке того и другого — буквально по паре патчей там и там, но хоть не надо разбираться, как скомпилировать и проверить (я точно знал о возможности сборки под VS2010).
Notepad++
86 файлов в проекте, 2 минуты на полную проверку. 48 сообщений о подозрительном коде от CppCat (в среднем это 0.55 сообщения на файл)
ToAscii(wParam,(lParam >> 16) && 0xff,keys,&dwReturnedValue,0); // V560. A part of conditional expression is always true/false.
WPARAM wParam
...
if(wParam<0) // V547. Expression is always true/false.
j=lstrlen(BGHS[SelfIndex].editstring);
BGHS[SelfIndex].editstring[j-1]=0x00; // V557. Array overrun is possible.
Тут вообще не факт, что это ошибка — возможно проверка строки на пустоту делается до этого, но мне кажется лучше не надеяться на случай.
lpcs = &cs;
lpcs = (LPCREATESTRUCT)lParam; // V519. The 'x' variable is assigned values twice successively. Perhaps this is a mistake.
if(MATCH)
{
return returnvalue+MAX_GRIDS;
}
if(!MATCH)) // V560. A part of conditional expression is always true/false.
{
return -1;
}
char *source = new char[docLength];
if (source == NULL) // V668. There is no sense in testing the pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error.
return;
По моей личной статистике — самая часто встречающаяся ошибка в любом коде на С++ (в этом проекте встречается 24 раза). Восходит своими корнями к функциям выделения памяти из языка С, которые возвращали NULL при ошибке. Но при использовании оператора new в С++ для проверки «а не закончилась ли доступная память?» нужно ловить исключение std::bad_alloc, а не проверять результат на NULL.
TCHAR intStr[5];
...
if (!intStr) // V600. Consider inspecting the condition. The 'Foo' pointer is always not equal to NULL.
Если уж в программе дело дойдет до того, что указатель на объявленный локально массив из пяти символов станет равным NULL — что-то пошло настолько плохо, что лучше уже просто упасть.
do
{
...
} while(openFound.success && (styleAt == SCE_H_DOUBLESTRING || styleAt == SCE_H_DOUBLESTRING) && searchStartPoint > 0);
Тут пытались найти кавычки (двойные и одинарные), а на самом деле дважды проверяют двойные. Копипаста с планом заменить вторую проверку на SCE_H_SINGLESTRING, о чём по ходу дело забылось. Один из самых полезных найденных багов — действительно баг в парсере XML, а не просто «совет по улучшению».
Ложные (на мой взгляд) срабатывания
Два подряд идущих блока if с одинаковым условием
//Setup GUI
if (!_beforeSpecialView.isPostIt) // V581. The conditional expressions of the 'if' operators situated alongside each other are identical.
{
...
}
//Set old style if not fullscreen
if (!_beforeSpecialView.isPostIt)
{
...
}
Тут я с CppCat не согласен. Мало ли почему программист решил так написать? Код валидный, работает хорошо. Это не может быть ошибка «забыли убрать !», потому что иначе тут было бы «else». Просто такое оформление кода.
Несколько ошибок «Priority of the '&&' operation is higher than that of the '||' operation»
printPage = (!(_pdlg.Flags & PD_PAGENUMS) ||
(pageNum >= _pdlg.nFromPage) && (pageNum <= _pdlg.nToPage));
CppCat упорно считает, что программисты не знают приоритетов операций. В данном случае и по смыслу и по переносам видно, что программист знал, что делает. Конечно, «явное лучше неявного», но ошибок тут нет.
Анализатор не предполагает, что бессмысленный код может иметь смысл с другими ключами компиляции
if (unicodeSupported)
::DispatchMessageW(&msg);
else // V523. The 'then' statement is equivalent to the 'else' statement
::DispatchMessage(&msg);
Это потому, что ранее написано "#define DispatchMessage DispatchMessageW". Но вот ведь в чём дело — эта замена включается макросами условной компиляции. В данном случае код и правда не имеет смысла, но если проект скомпилировать с другими ключами, то DispatchMessage будет указывать на DispatchMessageA, а значит код будет иметь смысл.
Я, конечно, придираюсь: несправедливо требовать от статического анализатора догадываться «какие там ещё могут быть опции компиляции». А вот от программисту подумать об этом не помешает.
Вывод по Notepad++
Из 48 сообщений об ошибках на мой взгляд около 38 действительно заслуживают внимания и некоторых правок в коде. Из них 30 мест являются явными багами, а 8 — полезными, но необязательными стилистические правками. 10 сообщений от CppCat я расцениваю как ложные в контексте логики кода. В целом — неплохо.
ZeroMq
72 файла, 1 минута на анализ.
Ровно 1 найденное подозрительное место. И то по факту — ложное.
rc = pipe_->write (&probe_msg_);
// zmq_assert (rc) is not applicable here, since it is not a bug.
pipe_->flush ();
rc = probe_msg_.close (); // V519. The 'x' variable is assigned values twice successively. Perhaps this is a mistake.
Анализатор расстроен тем фактом, что бедное значение rc никому не нужно между его первым и вторым присваиванием. Да, не нужно. Но ведь:
- Удобно при отладке поставить брейкпоинт и посмотреть чему оно равно.
- Есть коммент, указывающий что здесь была раньше (или могла бы быть) проверка rc. Также он говорит, что эта проверка точно не нужна.
Анализатор не обязан, конечно, читать комментарии, поэтому и не знает почему тут всё ок.
Выводы по ZeroMq
Ни одного полезного сообщения о подозрительном месте в коде. Ну что ж, я с самого начала сказал, что очень высокого мнения о коде этой библиотеки.
Мой проект
420 файлов (в 8-ми подпроектах), 9 минут на анализ, 99 предупреждений. В среднем это 0.23 предупреждения на файл.
void CHttpDownloaderBase::GetResponseHeader(const std::string& strHeaderName,
std::list<std::string>& listValues) const
{
listValues.empty(); // V530. The return value of function 'Foo' is required to be utilized.
...
Путаница из-за того, что в некоторых фреймворках метод empty контейнерного типа действительно очищает контейнер. Но вот для std::list — это лишь проверка пустоты. Нужно заменить на clear()
const char *xml_attr(xml_t* xml, const char *attr)
{
...
const char *name = xml->name;
if (! xml || ! xml->attr) // V595. The pointer was utilized before it was verified against nullptr.
return NULL;
while (*(n = ++s + strspn(s, XML_WS))) // V567. Undefined behavior. The variable is modified while being used twice between sequence points.
{...}
Вообще-то на компиляторе С++ от VS2010 этот код работает как задумывалось. Но формально — да, стандарт говорит «неопределённое поведение». Лучше исправить.
if (!text[++r] == '\') // V562. It's odd to compare a bool type value with a value of N.
{
break;
}
if (!text[++r] == 'u') // V562. It's odd to compare a bool type value with a value of N.
{
break;
}
Всё просто, оператор "!" имеет более высокий приоритет, чем "=="
TCHAR *ch = path + lstrlen(path) - 1; // V532. Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'.
while (*ch && *ch != '\')
*ch--;
Предположение CppCat о том, что я может быть хотел изменить значение по указателю — неверно, я хотел изменить сам указатель. Но тем ни менее польза от этого сообщения есть — оно указывает на лишнюю операцию взятия значения по указателю — "*". Оно здесь не используется, а значит можно написать просто «ch--»
while (*p1 != 0 && *p1 == _T(' ')) // V590. Consider inspecting this expression. The expression is excessive or contains a misprint.
p1++;
Всё просто — если сравнивать p1 с пробелом, то первая проверка не нужна.
m_dwInPartPos = 0;
m_pcOutData = NULL;
...
m_dwInPartPos = 0; // V519. The 'x' variable is assigned values twice successively.
if (type == eRT_unk) // V556. The values of different enum types are compared.
return false;
Одна из самых больших скорбей С++ (ну по крайней до появления enum classes в новом стандарте) — это то, что enum фактически является не отдельной сущностью, а набором чисел. Имея в одном enum значение eRt_unk, а во втором — eResourceUnknown мне просто повезло, что они оба были равны 0. Ошибка была в коде годы, хотя всё по чистому везению и работало.
if (scheme == ZLIB_COMPRESSION)
out.push(boost::iostreams::zlib_compressor());
else if (scheme = GZIP_COMPRESSION)
out.push(boost::iostreams::gzip_compressor()); // V559. Suspicious assignment inside the condition expression of 'if/while/for' operator.
Ну что тут скажешь — дурацкая ошибка, оправданий нет.
Ложные срабатывания
Выход за границы массива
int len = lstrlen(szLogDir);
TCHAR ch = szLogDir[len-1]; // V557. Array overrun is possible.
Действительно, переменная len не проверяется на ">0", но вот чуть выше по коду проверяется сама строка szLogDir на не-пустоту. Вторая проверка не добавит надёжности.
Подозрительные несрабатывания
Нашел у себя в коде такой вот кусочек:
if (m_packets[i] != NULL)
delete m_packets[i];
CppCat ничего о нём не сказал, хотя, по-моему, PVS-Studio в таких случаях говорила, что удалять NULL — безопасно.
Вывод по моему проекту
Из 99 предупреждений примерно 65 были по делу, распределение багов и просто «улучшалок» кода где-то 50/50.
Выводы
Плюсы
- Простота
- Цена
- Находит реальные баги
Минусы
- Отсутствие интеграции с VS2005VS2008
- Некоторый процент ложных срабатываний
Могло бы быть минусом, но нет!
Отсутствие поддержки 64-битных проверок.
Никогда не понимал, почему разработчики PVS-Studio всегда так концентрировали на них внимание. Под Windows x64 работают 32-битные сборки программ, а соответственно вопрос о создании 64-битной версии становится чаще всего или когда нужно писать драйвер, или когда программе нужно больше 3 Гб ОЗУ. В моей статистике это около 10-15% проектов.
Отсутствие интеграции с MsBuild и т.д.
Мне кажется логичным проверять код вручную, перед комитом в репозиторий. Не при каждом билде (медленно), не на билд-сервере (не интерактивно), а вот так. Времени уходит не много, перед коллегами не позоришься — удобно.
Могло бы быть плюсом, но нет!
Аскетичность в настройках
CppCat получился очень уж суровым. Не хватает гибкости: можно исключить из проверки файлы, папки, конкретную строку файла. Но как исключить часть файла? Как отключить проверку определённой ошибки или класса ошибок? А если еще и не глобально, а для части файлов? Это то ли невозможно, то ли я мало читал документацию.
Хотелки
Хотелка 1
Было бы классно иметь возможность хранить информацию об исключенных из проверки предупреждениях не в комментариях к коду, а где-то во внешнем файле, который можно было бы исключить из комита в репозиторий. Не все коллеги могут пользоваться тем же инструментом статического анализа, а читать в коде комментарии вида "//-V:is_test_ok&=:501" и удивляться их предназначению может раздражать.
Хотелка 2
Было бы неплохо в контекстное меню предупреждения добавить пункт «Скопировать», ну и хоткей Ctrl+V. Очень было бы удобно копипастить эти предупреждения в текст комментария к коммиту в репозиторий. Сейчас можно, конечно, открыть документацию и скопировать оттуда заголовок — но там текст обобщённый, а в результатах проверки указываются конкретные строки в коде, названия переменных — удобно и понятно.
Окончательные выводы
Что хорошо в CppCat — это простота подсчёта того, стоит ли инструмент покупки. Давайте возьмём среднюю зарплату среднего С++ программиста в вакууме из недавно пробегавшего на Хабре обзора: 80000 рублей (2370$). Значит 1 час его работы стоит примерно 14 долларов. Я думаю вы согласитесь, что найти и исправить баг, подобный описанным выше(а ещё если и не знаешь точно, что ищешь) — это минимум час работы. Стоимость CppCat — 250$. Это как 17 часов работы. Если в вашем проекте вы планируете потратить на багфиксинг больше 17 часов (а что, бывают проекты, где планируют меньше?), то покупка оправдана.
В общем, спасибо авторам CppCat за заботу о небольших проектах и индивидуальных разработчиках. Надеюсь на реализацию «хотелок» и интеграцию с VS2008.
Автор: tangro
