Разработка программного обеспечения на C++ с годами становится сложнее. Иногда, например, в отношении нового стандарта C++11 можно слышать, что язык усложнился. Да это так, но парадокс в том, что язык усложнился, а разработка на нем стала проще благодаря новым возможностям. И вся сложность разработки ПО на C++ не только в сложности языка, а в том, что с годами потребители от программного обеспечения ждут все больше возможностей. Как следствие растет и кодовая база проектов, для приведения которой в хороший законченный программный продукт требуется все больше вспомогательного ПО для разработчиков. Одним из таких инструментов является статический анализатор кода, который помогает разработчику найти проблемные или подозрительные участки кода, о которых компилятор промолчал.
Необходимость в большем количестве инструментов и статическом анализе возникает не просто так, не от лени и не от требований писать код еще быстрее. Чем больше и сложнее программа, тем больше в ней ошибок. Причем, количество дефектов растет быстрее, чем размер кода. Этот эффект описывал Стив Макконнелл в книге «Совершенный код» (см. также «Ощущения, которые подтвердились числами»). Так что без вспомогательных инструментов, берущих на себя часть работы, обходиться со временем все сложнее.
Не так давно появился новый такой инструмент — CppCat. О том как продукт PVS-Studio показал себя в проверке того или иного проекта с открытым кодом уже написано немало. PVS-Studio и CppCat базируются на одном и том же движке. Попробую отправить CppCat на поиски подозрительных мест в проектах, над которыми довелось поработать и из которых есть возможность показать небольшие фрагменты кода. Что из этого получилось?
Самый первый эксперимент меня впечатлил. Ранее я публиковал на Хабре статью «Своя компонентная модель на С++». Кода там немного и я был приятно удивлен, когда CppCat нашел три одинаковых ляпа, получившихся благодаря полуинтеллектуальному копированию из одного и того же фрагмента:
IEnumHelper(IEnumPtr enumerator)
: Enumerator(enumerator)
{
if (!Enumerator.Get())
IEnumHelperException("Empty IEnum pointer");
}
Здесь пропущен оператор throw. Ошибка, на которую указал анализатор: V596. The object was created but it is not being used. The 'throw' keyword could be missing.
Да, объект исключения создан, а вот кинуть его в качестве исключения я забыл. Как результат при ситуации, когда будет передан пустой указатель, конструктор его спокойно примет, а не должен. При исполнении программы, другие методы, которые полагаются, что все проверено в конструкторе, могут завершить неожиданно процесс, обратившись по нулевому указателю.
Не смотря на небольшой размер проекта и местами не самый простой код, мне иногда приходят сообщения с какими-то вопросами. Немного, но данной «поделкой» интересуются. И теперь эти ошибки исправлены благодаря проведенному анализу, а не наступив на них при использовании модели. А кто интересовался данной моделью могут воспользоваться кодом с исправленными ошибками.
Воодушевившись экспериментом, я решил проверить другой имеющийся у меня код. Результат меня не впечатлил. Вернее сказать, я испытал смешанные чувства. С одной стороны жаль, что мало полезных предупреждений, с другой стороны, приятно, что был написан аккуратный код. Такое впечатление, что нас (команду разработчиков этого проекта) кто-то похвалил за наш стиль программирования и подход в организации проекта.
Несколько лет назад мы разрабатывали один небольшой проект. За несколько месяцев в нем как-то накопилось около 16 Мб кода. В проекте было всего трое разработчиков. Кто в какой степени был в нем занят упущу. Главное — проект имеет неплохой размер для тестирования статического анализатора и есть возможность показать небольшие фрагменты кода.
Собираю проект на уже не совсем новом по текущим меркам ноутбуке. Сборка на MS Visual Studio 2010 заняла чуть менее 10 минут. А вот проверка анализатором кода заняла куда больше времени. После чего анализатор кода выдал некоторое немалое количество проблемных мест. Да, неужто так много косяков в проекте? Оказалось что нет. В проекте были пара сторонних открытых проектов, вокруг которых были написаны наши обертки. Эти самые сторонние проекты были не в самых современных тенденциях написаны, поэтому анализатор их весьма серьезно пожурил. Да и один из кусков для работы с SOAP сгенерированный некоторой тулзой для работы с wsdl показался очень подозрительным анализатору.
Чтобы исключить все лишнее в рассмотрении ошибок проекта, в настройках CppCat задал папки со сторонним кодом, включенным в проект, которые стоит исключить из анализа. Количество подозрительных мест в коде выданное анализатором стало заметно меньше.
Для исключения из анализа того или иного файла или целой папки, я в начале это делал через главное меню «студии», куда встроен пункт «CppCat». Оказалось, как всегда, все гениальное просто: в списке найденных подозрительных мест кликнув в меню по описанию проблемы правой кнопкой в меню оказался пункт для исключения файла с этой и иными в этом файле проблемными местами. В общем: все оказалось удобно. Просмотрел…
Проблемные места и разбор полетов
Первые подозрения были в этом фрагменте:
void IEnvironmentImpl::AddIFaceId(const std::string &ifaceId)
{
if (Interfaces.find(ifaceId) != Interfaces.end())
throw IEnvironmentImplException("IFace id already exists");
Interfaces[ifaceId];
}
Анализатор указывает проблему: V607. Ownerless expression 'Foo'.
Проблемная строка: “Interfaces[ifaceId];”
Смотрю куда тянется код
class IEnvironmentImpl ...
{
...
typedef Common::RefObjPtr<IFaces::IBase> IBasePtr;
typedef std::map<std::string, IBasePtr> IFacesPool;
mutable IFacesPool Interfaces;
};
Interfaces – это карта из строки-идентификатора интерфейса и умного указателя на объект с интерфейсом IBase. Казалось бы, подозрительное выражение и правда не имеет значения. Но это std::map и оператор “[ ]” добавит не существующее в карте значение с пустым объектом умного указателя. Возможно это место для того, чтобы задуматься о смене контейнера, кода по добавлению или вообще в целесообразности метода усомниться, но код корректен и никак это не бесполезное выражение. Просто, удалив этот «бессмысленный код», образуется дефект. C++ — язык позволяющий сделать одно и то же множеством способов…
Следующая проблема была найдена в одном из функторов, в его операторе ”( )”. И это уже был действительно код с запахом, хотя я могу понять причины написания такого кода.
bool operator() (const DataVector::value_type &left, const DataVector::value_type &right) const
{
bool Res;
if (IntCompare)
{
int Left = 0;
int Right = 0;
FromString(&Left, left.second[ColIndex]);
FromString(&Right, right.second[ColIndex]);
Res = Ascending ? Left < Right : Left > Right;
}
else
Res = Ascending ? left.second[ColIndex] < right.second[ColIndex] : left.second[ColIndex] > right.second[ColIndex];
return Res;
}
Анализатор указывает на строку “return Res;”.
Проблема: V614. Uninitialized variable 'Foo' used.
Казалось бы if/else написан неплохо и третьего варианта с проверкой не дано. Монеты в воздухе редко зависают, обычно или орел, или решка. Но все-таки не очень красиво написано. Тут можно бы вспомнить о правилах хорошего тона и изначально инициализировать переменную.
Да, переменную явно по правилам хорошего тона надо инициализировать, даже в таком простом случае, как приведен выше. Анализатор напомнил о правилах хорошего тона. Но все же о данной недоработке (по моему мнению) я написал разработчикам.
А вот такая проблема по мнению анализатора, для кода будет губительна, если ее решить по данным рекомендациям.
Проблема: V508. The use of 'new type(n)' pattern was detected. Probably meant: 'new type[n]'.
Проблемный код:
SelectedInstrument.Reset(new unsigned long(static_cast<unsignedlong>(InstrumentsList.GetItemData(Index))));
где
Common::SharedPtr<unsigned long> SelectedInstrument;
Что здесь происходит? В умном указателе хранится обычный интегральный тип. Может для анализатора это и странно, но логика была такова: если указатель пуст, то поле не участвует в логике расчета, если нет, то значение, хранимое под этим указателем, имеет свой вес. Эдакий аналог NULL в поле базы данных. Да, возможно это проблемное место для того, чтобы задуматься над самим подходом или использовать соответствующий примитив из boost. Но boost в этой части проекта не было. По причинам, которые не так важны для данного материала. И если поступить по рекомендации анализатора, то можно получить две проблемы.
Первая аналогичная такому коду:
int *p = new int[n];
…
delete p;
А вторая – вместо одного значения хранился бы массив мусора размером в переданное значение, который мог бы и не создаться по причине нехватки памяти, размером в некоторое значение, которое предполагалось хранить как число, а не массив. А другая часть логики выдавала бы значение равное первому из массива мусора вместо полученного значения.
Еще аналогичный кусок кода, где разработчик прав:
return Common::AutoPtr<unsigned long>(new unsigned long(Capacity));
Да, в приведенном коде стоило бы задуматься над подходом, но не слепо следовать рекомендациям. Такой подход был в нескольких местах; сообщений я получил немало.
Следующий проблемный код:
Common::SharedPtr<IFaces::DateTime> EndDate = GetView().ShowDateTimeEditor(AuctionEndDate.Get());;
if (EndDate.Get())
{
Common::RefObjPtr<IFacesImpl::IEnumImpl> Strings = varMap.Get(PropertyPtrValue);
if (!Strings.Get())
throw SellLogicException("Error get visible strings");
Strings->Clear();
AuctionEndDate = EndDate;
std::wstring DateString = TimeToString(*AuctionEndDate.Get());
…
}
Проблема: V595. The pointer was utilized before it was verified against nullptr.
Проблемное место в строке:
std::wstring DateString = TimeToString(*AuctionEndDate.Get());
Выше строкой только что присвоен проверенный указатель полю класса (AuctionEndDate = EndDate). И тут же его проверять? Указатели, конечно, стоит проверять перед их использованием, но, по моему мнению, с которым может быть многие и не согласны, доходить до параноидальных проверок в обозримом куске кода, где и так понятно, что оно никуда не пропало и указатель хорошо удерживается умным указателем, считаю излишним. Если только не гнаться за статистикой стрококода в системе контроля версий.
Этот кусок кода оказался предметом небольшой переписки с Andrey2008 разработчиком анализатора. Такой был получен ответ:
Все понятно. Анализатор не понял, что AuctionEndDate.Get() и EndDate.Get() это одно и тоже. В результате, анализатор «видит» код таким
if (xxx)
{
*AuctionEndDate.Get()
}
if (!AuctionEndDate.Get())
Ай, проверяем указатель AuctionEndDate.Get() уже после разыменования.
В общем, ложное срабатывание. Запутался анализатор.
А вот небольшая, но приятная мелочь, найденная анализатором:
m_bRecalcCaptionRect = FALSE;
m_rCaptionRect = r;
m_bRecalcCaptionRect = FALSE;
Проблема: V519. The 'x' variable is assigned values twice successively. Perhaps this is a mistake.
Тут особо криминала нет, кроме одного момента. Программист, внося наспех изменения в проект, меняет одну из повторяющихся строк, вторую не заметив, и решает, что он малой кровью отделался от небольшого изменения в чужом коде, которые от него хотели видеть. И, не проверяя, сразу отправляет код в программу контроля версий и запускает очередную сборку на сборочной машине, но потом недоумевает, почему не получилось с первого раза и тратит время на поиск проблемы. А иногда глаз замыливается, особенно далеко за концом рабочего дня конца рабочей недели. Вы никогда в конце недели, перерабатывая, не наступали на грабли написав что-то типа такого
MyLog("%s", 10);
в коде вашего сервера и долго не могли понять причин его падения на ровном месте? А начинать отладку уже не хочется в этот момент.
Так что мелочь, но приятно…
Еще проблемный код с небольшой избыточностью:
if (bHeader)
ClickToHeader(bTopHeader, bTopHeader ? nCol : nRow, bDblClik);
else if (!bHeader && nCol >= 0 && nRow >= 0)
Проблема: V560. A part of conditional expression is always true/false.
Согласен, что в ветке else-if опять проверять флаг bHeader не имеет смысла.
Анализатор в коде самописного Grid'а нашел пару разных методов с одинаковым телом:
void CGridCtrl::DrawLeftHeaderItemText(CDC &dc, RECT r, LPCTSTR lpItemText, int nTextLen, BOOL bSelected)
{
DrawHeaderItemText(dc, r, lpItemText, nTextLen, bSelected);
}
…
void CGridCtrl::DrawTopHeaderItemText(CDC &dc, RECT r, LPCTSTR lpItemText, int nTextLen, BOOL bSelected)
{
DrawHeaderItemText(dc, r, lpItemText, nTextLen, bSelected);
}
сказав свое “фи” таким образом: V524. It is odd that the body of 'Foo_1' function is fully equivalent to the body of 'Foo_2' function.
Да, стоило бы вынести в отдельный метод тело этих методов. Но что выносить? Там и так вызван один и тот же метод. Абстрактный интерфейс определяет разные методы для отрисовки того или иного элемента таблицы, но так как в данной реализации они одинаково отрисовываются, то тут уж нечего делать.
Комментарий Andrey2008 разработчика анализатора: «Да, это ложное срабатывание. А смутило то, что в именах функции есть Left и Top. Странно, что функции работающие с разными направлениями, реализованы одинаково. Не было бы Letf, Top, то и не было бы предупреждения.»
Да, как-то так получалось, что заголовок строки и заголовок столбца в данном проекте имели одинаковую отрисовку. Так оно и задумывалось.
Еще подозрение на неинициализированную переменную:
std::auto_ptr<IObjectHolder> Ret;
Common::WStringVector Lines;
...
if (Lines.empty())
return Ret;
В строке “return Ret;” есть проблема: V614. Uninitialized variable 'Foo' used.
Класс std::auto_ptr явно имеет конструктор по умолчанию, который инициализирует указатель. Писать тут явную инициализацию не вижу смысла. Ну не передавать же аналогично для сложных объектов все их параметры при наличии параметров по умолчанию.
Неточность кода тут в ином: переменная далеко объявлена от места ее использования. Реальное ее использование куда ниже, а в проблемном месте можно было обойтись кодом типа:
Common::WStringVector Lines;
...
if (Lines.empty())
return std::auto_ptr<IObjectHolder>();
Переменную же объявить непосредственно перед ее первым использованием или даже совместить эти два действия.
Еще одна схожая проблема:
Common::CharVectorPtr RetBmp(0);
if (!width || !*width || !height || !*height)
return RetBmp;
И решаться она должна, по моему убеждению, максимально близким объявлением переменной к месту ее использования, а не инициализацией. А в приведенном коде как раз переменная уже инициализирована, но анализатор упорно выдает: V614. Uninitialized variable 'Foo' used.
Это код уже действительно с “запахом”:
~SceneInserterProxy()
{
try
{
Ctrl.EndAdd();
}
catch (std::exception &)
{
Ctrl.DeleteFromQuarantine(Objects);
throw;
}
}
Проблема: V509. The 'throw' operator inside the destructor should be placed within the try..catch block. Raising exception inside the destructor is illegal.
Да, ловить проблемы возникшие в связи с исключениями в деструкторах бывает не самым тривиальным делом. Исключения в деструкторах — дурной тон. Но данный пример — это всего лишь жертва «копипаста» с иного куска кода, скопированного в деструктор. Паттерн «полуинтеллектуальный копипаст» :)
Подводим итоги
Анализатор CppCat нашел явные проблемы с не выброшенным исключением. Нашел очень сомнительное место с перебросом исключения в деструкторе, которое при наступлении этой маловероятной ситуации могло стать на некоторое, возможно немалое время, гейзенбагом. Выдал множество предупреждений, на основании которых можно сделать неплохой рефакторинг кода. Стоит заметить, что не всегда надо просто исправлять места с предупреждениями анализатора по его предложенным примерам. Вполне может быть надо провести более масштабные работы. Так же анализатор указал на место с проблемным кодом, предложив решение, которому не стоило следовать. Но место он выявил и дал повод поразмыслить о смене принятого подхода. Были и еще выявленные подозрительные места, но здесь я не стал их рассматривать из-за малого интереса к ним.
Ложные срабатывания… Можно на них закрыть глаза на фоне пользы от выявленных реальных ошибок. Да и анализатор вряд ли будет стоять на месте и не развиваться.
Не могу сказать, что анализатор CppCat шокировал меня найденными ошибками. Да, он полезен, что-то было найдено. Однако, не могу назвать эту статью рекламной, так как анализатор в тихом омуте проекта весьма суровых чертей не нашел.
Впрочем, я понимаю, что многие могут заявить: «Не совсем честно анализировать завершенный проект». Но это не так. Кто работал в проектах с кодом, разрабатываемым не одной сотней разработчиков и не одним поколением кочующих между компаниями тех же разработчиков, и который собирается не один час на сборочных серверах, тот вполне знаком с тем, что даже успешно продаваемый программный продукт в своем коде может содержать очень много не самого элегантного кода. Казалось бы, грамотные люди с хорошими навыками, прочитавшие много литературы по разработке, а делают весьма странные тривиальные ошибки. Все просто: его величество стрессовый момент именуемый релизом, когда ценится не элегантность кода, а его работоспособность в жесткий срок выхода.
Возможно, многих ошибок при написании и отладке кода можно было бы избежать за счет статического анализа и этим сэкономить время. Однажды один сотрудник компании, в которой мне когда-то довелось поработать, заметив книжку про расширенную отладку с помощью WinDbg на столе у другого сотрудника, сказал: «Нет чтобы писать хорошие программы, они читают как их изощренно отлаживать». Вот тут и встает дилемма: часть ошибок локализовать еще на стадии разработки за счет более дорогих сотрудников и больших трат на время разработки или тратить время (а время деньги и немалые в разработке) на весьма нетривиальный анализ образовавшихся дефектов…
Может есть и третий вариант: дополнительная верификация кода еще на этапе разработки соответствующим инструментом.
Итог: CppCat хорошо находит проблемные места, о которых промолчал компилятор, а вот как их исправить тут лучше подумать самому разработчику, а не всегда слепо следовать рекомендации. Исправив найденные проблемные места и явные ошибки, можно больше потратить времени на разработку функционала нежели на поиск и исправление ошибок в системе. Конечно, на долю самого разработчика останутся еще ошибки, однако, так хочется, чтобы их было как можно меньше…
P.S.
Демонстрационную версию CppCat можно использовать 7 суток чтобы успеть проверить весьма большой проект. Возможно, кто-то за это время даже успеет пропитаться некоторой симпатией к этому анализатору.
Дабы не вводить в заблуждение тех, кто посмотрел в мой профайл, хочу сказать, что я не сотрудник ООО «СиПроВер» и никогда им не был. Чему немного противоречит мой профайл, но это обусловлено тем, что я пишу в блог этой компании по договоренности с ее техническим директором.
Автор: NYM
