Главная
Блог разработчиков phpBB
 
+ 17 предустановленных модов
+ SEO-оптимизация форума
+ авторизация через соц. сети
+ защита от спама

О том что мне помог обнаружить CppCat в планах

Anna | 24.06.2014 | нет комментариев

Разработка программного обеспечения на C с годами становится труднее. Изредка, скажем, в отношении нового эталона C 11 дозволено слышать, что язык усложнился. Да это так, но парадокс в том, что язык усложнился, а разработка на нем стала проще вследствие новым вероятностям. И каждая трудность разработки ПО на C не только в трудности языка, а в том, что с годами покупатели от программного обеспечения ожидают все огромнее вероятностей. Как следствие растет и кодовая база планов, для приведения которой в отличный завершенный программный продукт требуется все огромнее вспомогательного ПО для разработчиков. Одним из таких инструментов является статический анализатор кода, тот, что помогает разработчику обнаружить проблемные либо подозрительные участки кода, о которых компилятор промолчал.

Надобность в большем числе инструментов и статическом обзоре появляется не легко так, не от лени и не от требований писать код еще стремительней. Чем огромнее и труднее программа, тем огромнее в ней ошибок. Причем, число недостатков растет стремительней, чем размер кода. Данный результат описывал Стив Макконнелл в книге «Идеальный код» (см. также «Ощущения, которые подтвердились числами»). Так что без вспомогательных инструментов, берущих на себя часть работы, обходиться со временем все труднее.

Не так давным-давно возник новейший такой инструмент — CppCat. О том как продукт PVS-Studio показал себя в проверке того либо другого плана с открытым кодом теснее написано много. PVS-Studio и CppCat основываются на одном и том же движке. Испробую отправить CppCat на искания подозрительных мест в планах, над которыми довелось поработать и из которых есть вероятность показать небольшие фрагменты кода. Что из этого получилось?

Самый 1-й эксперимент меня впечатлил. Ранее я публиковал на Прогре статью «Своя компонентная модель на С ». Кода там немножко и я был славно поражен, когда 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 на столе у иного работника, сказал: «Нет Дабы писать отличные программы, они читают как их изощренно отлаживать». Вот здесь и встает дилемма: часть ошибок локализовать еще на стадии разработки за счет больше дорогих работников и крупных трат на время разработки либо тратить время (а время деньги и немалые в разработке) на крайне нетривиальный обзор образовавшихся дефектов…
Может есть и 3-й вариант: добавочная верификация кода еще на этапе разработки соответствующим инструментом.

Вывод: CppCat отлично находит проблемные места, о которых промолчал компилятор, а вот как их поправить здесь отменнее подумать самому разработчику, а не неизменно слепо следовать рекомендации. Поправив обнаруженные проблемные места и очевидные ошибки, дозволено огромнее потратить времени на разработку функционала нежели на поиск и исправление ошибок в системе. Безусловно, на долю самого разработчика останутся еще ошибки, впрочем, так хочется, Дабы их было как дозволено меньше…

P.S.
Демонстрационную версию CppCat дозволено применять 7 суток Дабы поспеть проверить крайне огромный план. Допустимо, кто-то за это время даже поспеет пропитаться некоторой симпатией к этому анализатору.

чтобы не вводить в заблуждение тех, кто посмотрел в мой профайл, хочу сказать, что я не работник ООО «СиПроВер» и никогда им не был. Чему немножко противоречит мой профайл, но это обусловлено тем, что я пишу в блог этой компании по договоренности с ее техническим директором.

Источник: programmingmaster.ru
Оставить комментарий
Форум phpBB, русская поддержка форума phpBB
Рейтинг@Mail.ru 2008 - 2017 © BB3x.ru - русская поддержка форума phpBB