[Перевод] Как команда PVS-Studio улучшила код Unreal Engine

Наша компания создаёт, продвигает и продаёт статический анализатор кода PVS-Studio для C/C++ программистов. Однако, наше взаимодействие с клиентами не ограничивается исключительно продажей им лицензий на продукт PVS-Studio. Например, мы занимаемся некоторыми контрактными работами. В силу NDA обычно рассказать о них мы не можем, да и интересного рассказа не получится. Названия проектов, в которых мы принимаем участие, тоже ничего не скажут большинству наших читателей. Но в этот раз, название как раз говорит о многом. Мы поработали вместе с компанией Epic Games над проектом Unreal Engine. Об этом и будет наш рассказ.
Для продвижения статического анализатора кода PVS-Studio мы придумали интересный формат статей. Мы проверяем открытые проекты и пишем про найденные недочёты в коде. Вот здесь можно взглянуть на эти статьи: обновляемый список. От этих статей все в выигрыше. Читателям интересно смотреть на чужие ошибки. Заодно они узнают для себя новые способы как избежать этих ошибок, используя какие-то приёмы или стиль написания кода. Для нас польза в том, что больше людей узнаёт о существовании PVS-Studio. Для авторов проектов тоже польза — они могут исправить некоторые баги.

Одной из таких статей была "Долгожданная проверка Unreal Engine 4". Исходный код Unreal Endine отличался высоким качеством, но как известно, все проекты по разработке программного обеспечения содержат ошибки и PVS-Studio отлично справился с нахождением этих ошибок. Мы проверили проект и отправили результаты проверки в Epic Games. Авторы проекта сказали нам спасибо и поправили найденные нами дефекты. Но нам этого было мало, и мы решили постараться продать компании Epic Games лицензию на PVS-Studio.

Компания Epic Games была заинтересована в использовании PVS-Studio, чтобы улучшить свой движок. Таким образом, мы проверяем и правим исходники Unreal Engine с таким расчётом, чтобы в коде не осталось багов и при этом анализатор больше не выдавал ни одного ложного срабатывания. После этого они начнут использовать PVS-Studio на своей кодовой базе, сведя к минимуму сложность интеграции анализатора в свой процесс разработки. В этом случае Epic Games согласилась оплатить не только лицензию, но и дополнительно вознаградить нас за проделанную работу.

Мы согласились. Работа выполнена. И теперь предлагаем читателю познакомиться с интересными моментами, с которыми мы встретились в процессе работы c исходными кодами Unreal Engine.

Со стороны PVS-Studio в проекте участвовали Павел Еремеев, Святослав Размыслов, Антон Токарев. Со стороны Epic Games больше всего помощи мы получили от Andy Bayle и Dan O'Connor – без них наша работа была бы невозможна, спасибо!

Интеграция анализа PVS-Studio в сборку Unreal Engine

Для сборки в Unreal Engine используется собственная сборочная система – Unreal Build Tool. Имеется также набор скриптов для генерации проектных файлов для разных платформ и компиляторов. Так как PVS-Studio ориентирована в первую очередь на работу с компилятором Microsoft Visual C++, мы воспользовались скриптом для получения проектных файлов (*.vcxproj) для среды Microsoft Visual Studio.

PVS-Studio имеет плагин, встраивающийся в среду разработки Visual Studio и позволяющий выполнить анализ «в один клик». Однако, проекты, сгенерированные для Unreal Engine, не являются «обычными» проектами MSBuild, которые использует Visual Studio.

Во время компиляции Unreal Engine в Visual Studio, среда вызывает MSBuild при запуске сборки, однако, сам MSBuild используется лишь как «обёртка» для вызова упомянутого ранее Unreal Build Tool.

Для анализа исходного кода в PVS-Studio нужны результаты работы препроцессора. Анализатору требуется *.i файл в котором вставлены все заголовочные файлы и раскрыты все макросы.

Примечание. Далее этот раздел будет интересен только тем, кто использует нестандартную сборочную систему, например, как у Unreal Engine. Если вы планируете попробовать проверить с помощью PVS-Studio свой проект, который собирается как-то хитро, то предлагаю дочитать этот раздел до конца. Возможно, он поможет совладать и с проверкой вашего проекта. Если же у вас обыкновенный проект для Visual Studio, или хочется побыстрее почитать про найденные ошибки, то можно пропустить этот раздел.

Для того, чтобы корректно запустить препроцессор, необходима информация о параметрах компиляции. В «обычных» MSBuild проектах эта информация имеется, и плагин PVS-Studio «видит» её и может самостоятельно препроцессировать нужные исходники для последующего запуска анализатора. Иначе обстоят дела с проектами Unreal Engine.

Как уже было сказано выше, эти проекты – всего лишь «обёртка», реальный же вызов компилятора осуществляет Unreal Build Tool. Поэтому параметры компиляции и недоступны плагину PVS-Studio для Visual Studio. Запустить анализ «одним кликом» не получается, хотя плагин можно использовать для просмотра результатов анализа.

Непосредственно сам анализатор (PVS-Studio.exe) является command-line приложением, схожим по своему сценарию использования с C++ компилятором. Его, как и компилятор, необходимо запускать для каждого исходного файла, передавая параметры компиляции этого файла через командную строку или response файл. Анализатор же сам запустит правильный препроцессор и впоследствии выполнит анализ.

Примечание. Есть и другой вариант работы. Можно запустить анализатор и для уже готового препроцессированного файла.

Таким образом, универсальным решением для интеграции анализатора PVS-Studio будет вызов его исполняемого файла в том же месте, где происходит вызов компилятора – т.е. в сборочной системе. В данном случае в Unreal Build Tool. Понятно, что это потребует модификации используемой сборочной системы, что, как в нашем случае, может быть нежелательно. Поэтому, как раз для подобных ситуаций мы создали систему «перехвата» вызовов компилятора – Compiler Monitoring.

Система Compiler Monitoring способна «отлавливать» запуск процессов компиляции (в случае с Visual C++ это процессы cl.exe), собирать все необходимые для препроцессирования параметры у таких процессов, и перезапускать препроцессирование компилируемых файлов, с последующим запуском их анализа. Так и поступим.

Рисунок 1. Схематичное изображение процесса проверки проекта Unreal Engine

Интеграция анализа Unreal Engine сводится для нас к запуску непосредственно перед сборкой процесса мониторинга (CLMonitor.exe), который уже выполнит по завершении сборки все необходимые действия по препроцессированию и непосредственному вызову анализатора. Для запуска мониторинга нужно выполнить простую команду:

CLMonitor.exe monitor

CLMonitor.exe запустит сам себя в режиме отслеживания и завершится. При этом ещё один процесс CLMonitor.exe останется висеть в фоне, осуществляя непосредственное «отлавливание» компиляторов. По завершению сборки нужно выполнить ещё одну простую команду:

CLMonitor.exe analyze "UE.plog"

Теперь CLMonitor.exe запустит непосредственно анализ собранных ранее исходных файлов и сохранит результаты в файл UE.plog, с которым уже можно работать в нашем IDE плагине.

Мы настроили на своём Continuous Integration сервере регулярную ночную сборку интересных для нас конфигураций Unreal Engine с последующим запуском анализа. Таким образом мы, во-первых, проверяли, что наши правки не сломали сборку, а во-вторых, получали каждое утро новый отчёт о проверке Unreal Engine, с учётом всех заложенных в предыдущий день правок. И перед отправкой Pull Request'а о включении наших правок в основной репозиторий проекта, мы могли легко проверить, что текущая версия в нашем репозитории стабильна, просто перезапустив сборку на сервере.

Нелинейная скорость правок

Итак, мы совладали с процессом сборки проекта и его анализом. Теперь поговорим о правках в коде, на основе выдаваемых предупреждений.

На первый взгляд может показаться, что количество предупреждений, выдаваемых анализатором день ото дня, должно уменьшаться равномерно. Приблизительно одинаковое количество сообщений подавляется с помощью каких-то механизмов PVS-Studio, приблизительно одинаковое количество правок делается в коде.

То есть теоретически можно ожидать какой-то такой график:

Рисунок 2. Идеальный график. Количество ошибок уменьшается равномерно с каждым рабочим днём.

На самом деле, сообщения исчезают в начале быстрее, чем потом. Во-первых, на начальном этапе подавляются предупреждения, находящихся в макросах, и это быстро сокращает их количество. Во-вторых, получается так, что в начале исправляются очевидные вещи, а непонятные места откладываются на потом. Мы можем объяснить, почему в начале правились простые вещи. Хотелось продемонстрировать разработчикам из Epic Games, что мы приступили к работе и процесс пошел. Странно было бы начать со сложного и застрять на этом.

Всего непосредственно на работу с кодом Unreal Engine ушло 17 рабочих дней. Нашей целью было изничтожить все предупреждения первого и второго уровня общего назначения. Вот как продвигалась работа:

Таблица 1. Количество предупреждений анализатора в различные дни.

Обратите внимание на выделенные красным числа. В первые пару дней мы немного освоились, а потом подавили предупреждения в некоторых макросах, резко сократив количество ложных срабатываний.

Семнадцать рабочих дней это много и надо дать пояснения читателям. Во-первых, над проектом работала не вся команда, а только два человека. При этом они занимались и другим задачами. Во-вторых, код Unreal Engine нам совершенно не знаком, поэтому внесение правок было достаточно сложной задачей. Приходилось постоянно разбираться, стоит ли править код, и если да, то как.

Теперь те же самые данные представленные в виде сглаженного графика:

Рисунок 3. Сглаженный график количества предупреждений.

Практический вывод. Надо запомнить самим и других предупредить. Плохая идея делать оценку времени, которое понадобится для проработки всех предупреждений, только по первым дням работы. В первые дни работа идёт очень шустро и прогноз окажется слишком оптимистичный.

Но оценить как-то всё равно надо. Думаю, здесь хорошо подойдёт какая-то магическая формула и со временем мы её составим и покажем миру. Пока у нас недостаточно статистических данных, чтобы придумать что-то правдивое.

О найденных ошибках

Мы поправили достаточно много фрагментов кода. Правки условно можно разделить на 3 группы:

1. Настоящие ошибки. Ниже мы приведём несколько таких ошибок в качестве примера.
2. Беды не было, но код сбивал с толку анализатор и мог запутать человека, который начнёт изучать этот код. Другими словами, этот код, который «пахнет» и его тоже полезно поправить. Что мы и сделали.
3. Правки, которые вызваны исключительно потребностью «угодить» анализатору кода, выдающему ложные срабатывания. Мы старались по возможности вынести подавление ложных сообщений в отдельный специальный файл или улучшить работу самого анализатора. Но всё равно в нескольких местах пришлось провести рефакторинг, чтобы помочь анализатору понять, что к чему.

Как и обещали, посмотрим на некоторые примеры ошибки. Мы выбрали код попроще и поинтересней.

Первое интересное сообщение PVS-Studio: V506 Pointer to local variable 'NewBitmap' is stored outside the scope of this variable. Such a pointer will become invalid. fontcache.cpp 466

void GetRenderData(....)
{
  ....
  FT_Bitmap* Bitmap = nullptr;
  if( Slot->bitmap.pixel_mode == FT_PIXEL_MODE_MONO )
  {
    FT_Bitmap NewBitmap;
    ....
    Bitmap = &NewBitmap;
  }
  ....
  OutRenderData.RawPixels.AddUninitialized(
    Bitmap->rows * Bitmap->width );
  ....
}

Адрес объекта NewBitmap сохраняется в указателе Bitmap. Неприятность в том, что сразу после этого время жизни объекта NewBitmap заканчивается, и он разрушается. Получается, что Bitmap указывает на уже разрушенный объект.

При попытке использовать указатель для доступа к уничтоженному объекту возникнет неопределённое поведение. Как оно проявит себя — неизвестно. Программа даже может долгие годы успешно работать, если благодаря везению данные мертвого объекта (расположенные на стеке) не перетираются чем-то другим.

Правильно будет вынести объявление NewBitmap за пределы оператора 'if':

void GetRenderData(....)
{
  ....
  FT_Bitmap* Bitmap = nullptr;

  FT_Bitmap NewBitmap;
  if( Slot->bitmap.pixel_mode == FT_PIXEL_MODE_MONO )
  {
    FT_Bitmap_New( &NewBitmap );
    // Convert the mono font to 8bbp from 1bpp
    FT_Bitmap_Convert( FTLibrary, &Slot->bitmap, &NewBitmap, 4 );

    Bitmap = &NewBitmap;
  }
  else
  {
    Bitmap = &Slot->bitmap;
  }
  ....
  OutRenderData.RawPixels.AddUninitialized(
    Bitmap->rows * Bitmap->width );
  ....
}

Следующее предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'GEngine' might take place. Check the logical condition. gameplaystatics.cpp 988

void UGameplayStatics::DeactivateReverbEffect(....)
{
  if (GEngine || !GEngine->UseSound())
  {
    return;
  }
  UWorld* ThisWorld = GEngine->GetWorldFromContextObject(....);
  ....
}

Если указатель GEngine ненулевой, то происходит выход из функции. Всё хорошо. А вот если указатель GEngine нулевой, то он разыменовывается.

Мы исправили код следующим образом:

void UGameplayStatics::DeactivateReverbEffect(....)
{
  if (GEngine == nullptr || !GEngine->UseSound())
  {
    return;
  }

  UWorld* ThisWorld = GEngine->GetWorldFromContextObject(....);
  ....
}

Интересная опечатка ждёт читателей в следующем фрагменте кода. В нём анализатор обнаружил бессмысленный вызов функции: V530 The return value of function 'Memcmp' is required to be utilized. pathfollowingcomponent.cpp 715

int32 UPathFollowingComponent::OptimizeSegmentVisibility(
  int32 StartIndex)
{
  ....
  if (Path.IsValid())
  {
    Path->ShortcutNodeRefs.Reserve(....);
    Path->ShortcutNodeRefs.SetNumUninitialized(....);
  }
  FPlatformMemory::Memcmp(Path->ShortcutNodeRefs.GetData(),
                          RaycastResult.CorridorPolys,
                          RaycastResult.CorridorPolysCount *
                            sizeof(NavNodeRef));
  ....
}

Результат функции Memcmp не используется. Это и насторожило анализатор.

На самом деле, программист планировал скопировать участок памяти с помощью функции Memcpy(), но допустил опечатку. Исправленный вариант кода:

int32 UPathFollowingComponent::OptimizeSegmentVisibility(
  int32 StartIndex)
{
  ....
  if (Path.IsValid())
  {
    Path->ShortcutNodeRefs.Reserve(....);
    Path->ShortcutNodeRefs.SetNumUninitialized(....);

    FPlatformMemory::Memcpy(Path->ShortcutNodeRefs.GetData(),
                            RaycastResult.CorridorPolys,
                            RaycastResult.CorridorPolysCount *
                              sizeof(NavNodeRef));
  }
  ....
}

Поговорим о диагностическом сообщении, которое можно встретить при проверке практически любого проекта. Уж очень распространённый тип ошибки она выявляет. Речь идёт о диагностике V595. В нашей базе ошибок, она занимает первое место по количеству найденных недочётов (см. примеры). На первый взгляд, список не такой уж большой, по сравнению, скажем, с V501. Но дело в том, что диагностики V595 несколько скучны, чтобы выписывать много примеров из какого-то проекта. Часто выписан один пример и сделана приписка вида: And 161 additional diagnostic messages. В половине случаев — это самые настоящие ошибки. Вот как это выглядит:

Рисунок 4. Ужасы диагностики V595.

Диагностика V595 находит участки кода, где выполняется разыменование указателя до его проверки на ноль. Обычно в проверяемых проектах всегда находится некоторое количество таких предупреждений. Проверка и разыменование указателя могут находится довольно далеко в коде функции: в десятках или даже сотнях строк друг от друга, что усложняет исправление ошибки. Но попадаются и маленькие, очень наглядные, примеры, как, например, эта функция:

float SGammaUIPanel::OnGetGamma() const
{
  float DisplayGamma = GEngine->DisplayGamma;
  return GEngine ? DisplayGamma : 2.2f;
}
Предупреждение: V595 The 'GEngine' pointer was utilized before it was verified against nullptr. Check lines: 47, 48. gammauipanel.cpp 47

Мы исправили эту функцию следующим образом:

float SGammaUIPanel::OnGetGamma() const
{
  return GEngine ? GEngine->DisplayGamma : 2.2f;
}

Перейдем к следующему фрагменту:

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 289, 299. automationreport.cpp 289

void FAutomationReport::ClustersUpdated(const int32 NumClusters)
{
  ...
  //Fixup Results array
  if( NumClusters > Results.Num() )         //<==
  {
    for( int32 ClusterIndex = Results.Num();
         ClusterIndex < NumClusters; ++ClusterIndex )
    {
      ....
      Results.Add( AutomationTestResult );
    }
  }
  else if( NumClusters > Results.Num() )    //<==
  {
    Results.RemoveAt(NumClusters, Results.Num() - NumClusters);
  }
  ....
}

В текущем виде второе условие никогда не выполняется. Логично предположить, что ошибка в знаке второго условия, чтобы из массива 'Result' удалялись лишние элементы:

void FAutomationReport::ClustersUpdated(const int32 NumClusters)
{
  ....
  //Fixup Results array
  if( NumClusters > Results.Num() )
  {
    for( int32 ClusterIndex = Results.Num();
         ClusterIndex < NumClusters; ++ClusterIndex )
    {
      ....
      Results.Add( AutomationTestResult );
    }
  }
  else if( NumClusters < Results.Num() )
  {
    Results.RemoveAt(NumClusters, Results.Num() - NumClusters);
  }
  ....
}

Пример кода на внимательное чтение. Сообщение: V616 The 'DT_POLYTYPE_GROUND' named constant with the value of 0 is used in the bitwise operation. pimplrecastnavmesh.cpp 2006

/// Flags representing the type of a navigation mesh polygon.
enum dtPolyTypes
{
  DT_POLYTYPE_GROUND = 0,
  DT_POLYTYPE_OFFMESH_POINT = 1,
  DT_POLYTYPE_OFFMESH_SEGMENT = 2,
};

uint8 GetValidEnds(...., const dtPoly& Poly)
{
  ....
  if ((Poly.getType() & DT_POLYTYPE_GROUND) != 0)
  {
    return false;
  }
  ....
}

На первый взгляд всё хорошо. Кажется, что по маске выделяется какой-то бит и проверяется его значение. Но, на самом деле, в перечислении 'dtPolyTypes' определяются просто именованные константы, не предназначенные для выделения определённых бит.

В данном условии константа DT_POLYTYPE_GROUND равна 0, а значит условие никогда не выполнится.

Исправленный вариант:

uint8 GetValidEnds(...., const dtPoly& Poly)
{
  ....
  if (Poly.getType() == DT_POLYTYPE_GROUND)
  {
    return false;
  }
  ....
}

Выявленная опечатка: V501 There are identical sub-expressions to the left and to the right of the '||' operator: !bc.lclusters ||!bc.lclusters detourtilecache.cpp 687

dtStatus dtTileCache::buildNavMeshTile(....)
{
  ....
  bc.lcset = dtAllocTileCacheContourSet(m_talloc);
  bc.lclusters = dtAllocTileCacheClusterSet(m_talloc);
  if (!bc.lclusters || !bc.lclusters)   //<==
    return status;
  status = dtBuildTileCacheContours(....);
  ....
}

При копировании переменной, её забыли переименовать из 'bc.lclusters' в 'bc.lcset'.

Результаты регулярной проверки

Выше были перечислены далеко не все найденные ошибки, а только небольшая их часть. Мы привели их, чтобы продемонстрировать, какие ошибки может найти PVS-Studio даже в рабочем и оттестированном коде.

Однако, мы не устаём повторять, что разовая проверка кода — это неправильный способ использовать статический анализатор. Анализ нужно проводить регулярно и тогда огромное количество ошибок и опечаток можно найти ещё на этапе написания кода, а не на этапе тестирования или сопровождения.

Сейчас мы отлично подкрепим свои слова с помощью проекта Unreal Engine.

В начале мы правили код, не обращая внимания, свежий это код или старый. Было не до того. Зато мы очень хорошо заметили, как анализатор PVS-Studio начал выявлять ошибки в новом или исправленном коде, когда мы свели количество предупреждений до 0.

На самом деле, мы возились с кодом немного больше, чем 17 рабочих дней. Когда мы закончили вносить правки, и анализатор начал выдавать 0 предупреждений, мы в течение ещё 2 дней ждали, когда разработчики Unreal Engine примут наш последний Pull Request. И в течение этого времени продолжали обновляться из основного репозитория и выполнять проверку кода.

И за эти два дня мы успели увидеть, как анализатор обнаруживал ошибки в новом коде. Эти ошибки мы тоже поправили. Но самое главное в том, что это отлично демонстрирует пользу от регулярных проверок.

Фактически, конец графика «количество сообщений» приобрёл вид:

Рисунок 5. Схематичный график роста количества предупреждений, после того как их стало 0.

Давайте посмотрим, что мы успели обнаружить в последние два дня, проверяя свежие изменения в коде проекта.

День первый

Первое сообщение анализатора: V560 A part of conditional expression is always true: FBasicToken::TOKEN_Guid. k2node_mathexpression.cpp 235

virtual FString ToString() const override
{
  if (Token.TokenType == FBasicToken::TOKEN_Identifier ||
      FBasicToken::TOKEN_Guid) //<==
  {
    ....
  }
  else if (Token.TokenType == FBasicToken::TOKEN_Const)
  {
    ....
}

Забыли дописать «Token.TokenType ==». В результате, поскольку именованная константа 'FBasicToken::TOKEN_Guid' не равна 0, условие всегда истинно.

Второе сообщение анализатора: V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] CompressedDataRaw;'. crashupload.cpp 222

void FCrashUpload::CompressAndSendData()
{
  ....
  uint8* CompressedDataRaw = new uint8[BufferSize];         //<==

  int32 CompressedSize = BufferSize;
  int32 UncompressedSize = UncompressedData.Num();
  ....
  // Copy compressed data into the array.
  TArray<uint8> CompressedData;
  CompressedData.Append( CompressedDataRaw, CompressedSize );
  delete CompressedDataRaw;                                 //<==
  CompressedDataRaw = nullptr;
  ....
}

На практике эта ошибка не всегда проявляет себя, так как выделяется массив элементов типа char. Но всё равно это ошибка, которая приводит к неопределённому поведению и её следует обязательно исправить.

День второй

Первое предупреждение анализатора: V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. unrealaudiodevicewasapi.cpp 128

static void GetArrayOfSpeakers(....)
{
  Speakers.Reset();
  uint32 ChanCount = 0;
  // Build a flag field of the speaker outputs of this device
  for (uint32 SpeakerTypeIndex = 0;
       SpeakerTypeIndex < ESpeaker::SPEAKER_TYPE_COUNT,    //<==
       ChanCount < NumChannels; ++SpeakerTypeIndex)
  {
    ....
  }

  check(ChanCount == NumChannels);
}

Хорошая такая, жирная ошибка.

Оператор запятая ',' используется для выполнения стоящих по обе стороны от него выражений в порядке слева направо и получения значения правого выражения.

В результате, условием остановки цикла является только эта проверка: ChanCount < NumChannels.

Исправленное условие:

static void GetArrayOfSpeakers(....)
{
  Speakers.Reset();
  uint32 ChanCount = 0;
  // Build a flag field of the speaker outputs of this device
  for (uint32 SpeakerTypeIndex = 0;
       SpeakerTypeIndex < ESpeaker::SPEAKER_TYPE_COUNT &&
       ChanCount < NumChannels; ++SpeakerTypeIndex)
  {
    ....
  }
  check(ChanCount == NumChannels);
}

Второе предупреждение. V543 It is odd that value '-1' is assigned to the variable 'Result' of HRESULT type. unrealaudiodevicewasapi.cpp 568

#define S_OK       ((HRESULT)0L)
#define S_FALSE    ((HRESULT)1L)

bool
FUnrealAudioWasapi::OpenDevice(uint32 DeviceIndex,
                               EStreamType::Type StreamType)
{
  check(WasapiInfo.DeviceEnumerator);

  IMMDevice* Device = nullptr;
  IMMDeviceCollection* DeviceList = nullptr;
  WAVEFORMATEX* DeviceFormat = nullptr;
  FDeviceInfo DeviceInfo;
  HRESULT Result = S_OK;                      //<==
  ....
  if (!GetDeviceInfo(DataFlow, DeviceIndex, DeviceInfo))
  {
    Result = -1;                              //<==
    goto Cleanup;
  }
  ....
}

HRESULT — это 32-разрядное значение, разделенное на три различных поля: код серьезности ошибки, код устройства и код ошибки. Для работы со значением HRESULT служат специальные константы, такие как S_OK, E_FAIL, E_ABORT и так далее. А для проверки значений типа HRESULT предназначены такие макросы как SUCCEEDED, FAILED.

Предупреждение V543 выдается в том случае, если в переменную типа HRESULT пытаются записать значение -1, true или false.

Запись значения "-1" некорректна. Если хочется сообщить о какой-то непонятной ошибке, то следует использовать значение 0x80004005L (Unspecified failure). Эта и аналогичные константы описаны в «WinError.h».

Эх, какое сложное внедрение...

Некоторые программисты и менеджеры могут опечалиться, узнав, что для интеграции статического анализа в их проект потребуется N дней. Однако, не обязательно идти этим путём. Надо понимать, что разработчики Epic Games выбрали ИДЕАЛЬНЫЙ ПУТЬ, но не самый простой и быстрый.

Да, в идеале лучше сразу поправить все ошибки и потом быстро реагировать на новые сообщения анализатора, выдаваемые на новый код. Но можно начать получать пользу от статического анализа, не тратя время на правку старого кода.

Для этого PVS-Studio предлагает специальный механизм для разметки сообщений. В двух словах опишем общую идею.

Все сообщения, которые выдает анализатор, помечаются в специальной базе как неактивные. И тогда программисты видят только сообщения анализатора, относящиеся к новому или изменённому коду. То есть можно сразу начать получать пользу от анализа. А уже потом, если будет желание и время, можно потихоньку просматривать сообщения, относящиеся к старому коду и править недочёты.

Подробно про всё это можно прочитать здесь: документация, как быстро внедрить анализ в проект.

«А вы отписали разработчикам?»

После каждой статьи о проверке какого-нибудь проекта нас спрашивают: «А вы сообщили разработчикам?» И хотя мы всегда сообщаем, такие вопросы изрядно утомляют. Однако, в этот раз мы не только «отписали разработчикам», но и исправили все эти ошибки. В чем любой желающий может убедиться в репозитории на GitHub.

Заключение

Надеемся, проделанная работа по правке и улучшению кода Unreal Engine понравилась разработчикам Epic Games и принесла пользу проекту. И ждем новых проектов на движке Unreal Engine.

Некоторые выводы, которые можно сделать по результатам нашей работы:

1. Код проекта Unreal Engine весьма качественный. Пусть читателей не смущает большое количество предупреждений на начальном этапе. Это нормальная ситуация. Большинство этих предупреждений было убрано с помощью различных методов и настроек. Количество настоящих ошибок, обнаруженных в коде, для проекта такого размера, очень невелико.
2. Править чужой незнакомый код часто очень сложно. Впрочем, это и так понятно любому разработчику на интуитивном уровне. Тем не менее решили это отметить.
3. Скорость «переработки» предупреждений не линейна. Она будет замедляться и это надо учитывать при расчёте, сколько ещё осталось до конца правок.
4. Максимальная польза от статического анализа может быть получена только при его регулярном использовании.

Спасибо всем, кто прочитал эту статью и желаем всем безбажного кода. С уважением, разработчики анализатора PVS-Studio. И сейчас самое время его скачать и попробовать на своём проекте.

This entry passed through the Full-Text RSS service - if this is your content and you're reading it on someone else's site, please read the FAQ at http://ift.tt/jcXqJW.