# Единорог, который смог Одна из команд разработчиков Microsoft уже использует в работе анализатор PVS\-Studio\. Это хорошо, но недостаточно\. Поэтому я продолжаю демонстрировать, какую пользу может приносить статический анализ кода на примере проектов Microsoft\. Три года назад мы проверяли проект Casablanca и не смогли в нём ничего обнаружить\. За это проект был отмечен медалью "безбажный код"\. Прошло время, проект развивался и рос\. В свою очередь, анализатор PVS\-Studio существенно продвинулся в возможностях анализа кода\. И наконец я могу написать статью об ошибках, которые анализатор выявляет в проекте Casablanca \(C\+\+ REST SDK\)\. Ошибок мало, но то, что теперь их достаточно для написания статьи, говорит о эффективности PVS\-Studio\. ![0406_The_Little_Unicorn_That_Could_ru/image1.png](https://import.viva64.com/docx/blog/0406_The_Little_Unicorn_That_Could_ru/image1.png) ## Casablanca Как я уже отметил в аннотации, мы уже проверяли проект Casablanca\. Вы можете прочитать про это в статье "[Маленькая заметка о проекте Casablanca](https://pvs-studio.ru/ru/blog/posts/0189/)"\. [Casablanca \(C\+\+ REST SDK\)](https://github.com/Microsoft/cpprestsdk) \- это небольшой проект, написанный на современном C\+\+\. Говоря о современном языке C\+\+, я имею в виду, что в коде активно используется семантика перемещения \(move semantics\), лямбда\-функции, auto и так далее\. Новые возможности языка C\+\+ позволяют писать более короткий и надёжный код\. Это подтверждается тем, что найти ошибки в этом проекте непростая задача\. Хотя обычно мы [делаем](https://pvs-studio.ru/ru/blog/inspections/) это крайне легко\. Для заинтересовавшихся, какие ещё проекты Microsoft мы проверяли, привожу список статей, посвященных этим проверкам: [Xamarin\.Forms](https://pvs-studio.ru/ru/blog/posts/csharp/0400/), [CNTK](https://pvs-studio.ru/ru/blog/posts/cpp/0372/), [Microsoft Edge](https://pvs-studio.ru/ru/blog/posts/cpp/0370/), [CoreCLR](https://pvs-studio.ru/ru/blog/posts/0398/), [Windows 8 Driver Samples](https://pvs-studio.ru/ru/blog/posts/0398/), библиотека Visual C\+\+ [2012](https://pvs-studio.ru/ru/blog/posts/cpp/0163/) / [2013](https://pvs-studio.ru/ru/blog/posts/cpp/0288/), [CoreFX](https://pvs-studio.ru/ru/blog/posts/csharp/0365/), [Roslyn](https://pvs-studio.ru/ru/blog/posts/csharp/0363/), [Microsoft Code Contracts](https://pvs-studio.ru/ru/blog/posts/csharp/0361/), и скоро появится статья про проверку WPF Samples\. Итак, проект Casablanca является образцом хорошего, качественного кода\. Давайте посмотрим, что же все\-таки можно найти в нём с помощью статического анализатора кода PVS\-Studio\. ## Что удалось найти плохого **Фрагмент N1: опечатка** Имеется структура _NumericHandValues_, содержащая два члена: _low_ и _high_\. Вот объявление этой структуры: ```cpp struct NumericHandValues { int low; int high; int Best() { return (high < 22) ? high : low; } }; ``` А теперь посмотрим, как в одном месте инициализируется эта структура: ```cpp NumericHandValues GetNumericValues() { NumericHandValues res; res.low = 0; res.low = 0; .... } ``` Предупреждение PVS\-Studio: V519 The 'res\.low' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 130, 131\. BlackJack\_Client140 messagetypes\.h 131 Как видите, случайно два раза инициализировали член _low_, но при этом забыли инициализировать _high_\. Глубокомысленный комментарий здесь написать сложно\. Просто никто не застрахован от опечаток\. **Фрагмент N2: ошибка освобождения памяти** ```cpp void DealerTable::FillShoe(size_t decks) { std::shared_ptr ss(new int[decks * 52]); .... } ``` Предупреждение PVS\-Studio: V554 Incorrect use of shared\_ptr\. The memory allocated with 'new \[\]' will be cleaned using 'delete'\. BlackJack\_Server140 table\.cpp 471 По умолчанию умный указатель типа _shared\_ptr_ для уничтожения объекта вызовет оператор _delete_ без квадратных скобок _\[\]_\. В данном случае это неправильно\. Корректный вариант кода должен быть таким: ```cpp std::shared_ptr ss(new int[decks * 52], std::default_delete()); ``` **Фрагмент N3: потерянный указатель** Статический член _s\_server\_api_ представляет собой умный указатель и определён следующим образом: ```cpp std::unique_ptr http_server_api::s_server_api((http_server*)nullptr); ``` Подозрение вызывает код следующей функции: ```cpp void http_server_api::unregister_server_api() { pplx::extensibility::scoped_critical_section_t lock(s_lock); if (http_server_api::has_listener()) { throw http_exception(_XPLATSTR("Server API ..... attached")); } s_server_api.release(); } ``` Предупреждение PVS\-Studio: V530 The return value of function 'release' is required to be utilized\. cpprestsdk140 http\_server\_api\.cpp 64 Обратите внимание на "s\_server\_api\.release\(\);"\. После вызова функции release умный указатель больше не владеет объектом\. Указатель на объект "теряется" и объект будет существовать до конца жизни программы\. Скорее всего, мы вновь столкнулись с опечаткой\. Я думаю, что хотели вызвать функцию [_reset_](https://en.cppreference.com/w/cpp/memory/unique_ptr/reset), а вовсе не [_release_](https://en.cppreference.com/w/cpp/memory/unique_ptr/release)\. **Фрагмент N4: не тот enum** В проекте имеется два перечисления _BJHandState_ и _BJHandResult_, объявленные следующим образом: ```cpp enum BJHandState { HR_Empty, HR_BlackJack, HR_Active, HR_Held, HR_Busted }; enum BJHandResult { HR_None, HR_PlayerBlackJack, HR_PlayerWin, HR_ComputerWin, HR_Push }; ``` А теперь посмотрим на фрагмент кода из функции _PayUp_: ```cpp void DealerTable::PayUp(size_t idx) { .... if ( player.Hand.insurance > 0 && Players[0].Hand.state == HR_PlayerBlackJack ) { player.Balance += player.Hand.insurance*3; } .... } ``` Предупреждение PVS\-Studio: V556 The values of different enum types are compared\. Types: BJHandState, BJHandResult\. BlackJack\_Server140 table\.cpp 336 Переменная _state_ имеет тип _BJHandState_\. А это значит, что программист запутался в перечислениях\. По всей видимости код должен был выглядеть так: ```cpp if ( player.Hand.insurance > 0 && Players[0].Hand.state == HR_BlackJack ) ``` Забавно то, что это ошибка на самом деле пока никак не сказывается на работе программы\. Благодаря счастливому стечению обстоятельств, на данный момент константы _HR\_BlackJack_ и _HR\_PlayerBlackJack_ имеют одинаковое значение, равное 1\. Дело в том, что обе эти константы в перечислении находятся на одной позиции в списке\. Однако, в процесс развития программы это может измениться, и тогда возникнет странная, непонятная ошибка\. **Фрагмент N5: странный break** ```cpp web::json::value AsJSON() const { .... int idx = 0; for (auto iter = cards.begin(); iter != cards.end();) { jCards[idx++] = iter->AsJSON(); break; } .... } ``` Предупреждение PVS\-Studio: V612 An unconditional 'break' within a loop\. BlackJack\_Client140 messagetypes\.h 213 Оператор _break_ в этом коде выглядит крайне подозрительно\. Цикл может совершить максимум одну итерацию\. Мне сложно сказать, как должен вести себя этот код, но, скорее всего, сейчас с ним что\-то не так\. ## Прочие мелочи Помимо фрагментов кода, рассмотренных выше и претендующих на звание ошибок, анализатор обнаружил также несколько неаккуратных мест\. Например, использование постинкремента для итераторов\. ```cpp inline web::json::value TablesAsJSON(...., std::shared_ptr> &tables) { web::json::value result = web::json::value::array(); size_t idx = 0; for (auto tbl = tables.begin(); tbl != tables.end(); tbl++) { result[idx++] = tbl->second->AsJSON(); } return result; } ``` Предупреждение PVS\-Studio: V803 Decreased performance\. In case 'tbl' is iterator it's more effective to use prefix form of increment\. Replace iterator\+\+ with \+\+iterator\. BlackJack\_Client140 messagetypes\.h 356 Это, конечно, не ошибка\. Однако, хорошим стилем считается использование преинкремента: _\+\+tbl_\. Для тех, кто сомневается, что в этом есть смысл, я отсылаю к следующим двум статьям: 1. [Есть ли практический смысл использовать для итераторов префиксный оператор инкремента \+\+it, вместо постфиксного it\+\+](https://pvs-studio.ru/ru/blog/posts/cpp/0093/)\. 1. [Pre vs\. post increment operator \- benchmark](https://silviuardelean.ro/2011/04/20/pre-vs-post-increment-operator/)\. В коде библиотеки есть ещё 10 мест, где используется постинкремент итераторов в циклах, но я не вижу смысла приводить их в статье\. Рассмотрим ещё один пример, когда анализатор указывает на неаккуратный код: ```cpp struct _acquire_protector { _acquire_protector(....); ~_acquire_protector(); size_t m_size; private: _acquire_protector& operator=(const _acquire_protector&); uint8_t* m_ptr; concurrency::streams::streambuf& m_buffer; }; ``` Предупреждение PVS\-Studio: V690 The '\=' operator is declared as private in the '\_acquire\_protector' class, but the default copy constructor will still be generated by compiler\. It is dangerous to use such a class\. cpprestsdk140\.uwp\.staticlib fileio\_winrt\.cpp 825 Как видим, программист запретил использование оператора копирования\. Однако, объект по\-прежнему может быть скопирован с помощью конструктора копирования, создаваемого компилятором по умолчанию\. ## Заключение Наконец\-то анализатор PVS\-Studio смог к чему\-то придраться\. Ошибок нашлось немного, но всё\-таки они есть\. А это значит, что если применять статический анализ не разово, как я сделал сейчас, а регулярно, то можно предотвращать множество ошибок на самом раннем этапе\. Лучше править ошибки сразу после написания кода, а не в процессе тестирования, отладки и тем более после того как о дефекте сообщит один из пользователей\. ## Дополнительные ссылки 1. Название статьи является отсылкой к сказке "[Паровозик, который смог](https://ru.wikipedia.org/wiki/%D0%9F%D0%B0%D1%80%D0%BE%D0%B2%D0%BE%D0%B7%D0%B8%D0%BA,_%D0%BA%D0%BE%D1%82%D0%BE%D1%80%D1%8B%D0%B9_%D1%81%D0%BC%D0%BE%D0%B3)"\. 1. Ссылка, где вы можете [скачать](https://pvs-studio.ru/ru/pvs-studio/download/) анализатор PVS\-Studio и попробовать проверить один из своих проектов на языке C, C\+\+ или C\#\.