Спасибо, Марио, но код стоит поправить – проверка TheXTech

Здорово, когда энтузиастам-разработчикам удаётся сделать работающий клон известной игры. Ещё лучше, когда находятся люди, готовые продолжить развитие таких проектов! В этой статье с помощью PVS-Studio мы проверим TheXTech – открытую реализацию игры по вселенной Super Mario.

О проекте

TheXTech – это полностью переписанный на C++ игровой движок SMBX 1.3. Оригинальный SMBX (Super Mario Bros. X) был написан на Visual Basic 6 Эндрю Спинксом в 2009 году. Он позволял создавать уровни из элементов игр Nintendo серии Super Mario Bros. TheXTech умеет достаточно точно воспроизводить поведение оригинала с опционально включаемыми исправлениями его багов. Может запускаться не только под Windows, но и на macOS, Linux-системах с процессорами x86, ARM или PowerPC, также его портируют на 3DS и PS Vita.

Разработчик TheXTech – Виталий Новичков (Wohlstand) – подробно описал процесс разработки на Хабре и то, какие техники можно использовать для сглаживания различий при переводе с языка VB6 на C++. На странице GitHub опубликован дисклеймер (англ), поясняющий, почему исходный код находится не в самом лучшем состоянии: потому что оригинал состоит из лютого спагетти-кода, фрагменты которого мы сейчас и увидим.

Результаты проверки

Чистка кода

Фрагмент N1

Итак, можно ли здесь увидеть ошибку, которую нашёл анализатор?

V547 Expression 'NPC[A].Type == 54 && NPC[A].Type == 15' is always false. Probably the '||' operator should be used here. thextech npc_update.cpp 1277

Конечно же, нет :) Она находится посреди строки с условием длиной 1400 символов, и вам придётся пролистать 5 экранов вправо чтобы до неё добраться. Переформатируем код для наглядности:

else if(
     NPC[A].Type == 21 || NPC[A].Type == 22 || NPC[A].Type == 25
  || NPC[A].Type == 26 || NPC[A].Type == 31 || NPC[A].Type == 32
  || NPC[A].Type == 238 || NPC[A].Type == 239 || NPC[A].Type == 35
  || NPC[A].Type == 191 || NPC[A].Type == 193
  || (NPC[A].Type == 40 && NPC[A].Projectile == true) || NPC[A].Type == 49
  || NPC[A].Type == 58 || NPC[A].Type == 67 || NPC[A].Type == 68
  || NPC[A].Type == 69 || NPC[A].Type == 70
  || (NPCIsVeggie[NPC[A].Type] && NPC[A].Projectile == false)
  || (NPC[A].Type == 29 && NPC[A].Projectile == true)

  ||    (NPC[A].Projectile == true
     && (NPC[A].Type == 54 && NPC[A].Type == 15))            // <=

  || .... )
{ .... }

Разумеется, переменная NPC[A].Type не может одновременно равняться двум разным значениям. По всей видимости, логика условия должна была выполняться в том числе и для снарядов с типами 54 и 15, но сейчас эта часть условия всегда ложна. По всей видимости, нужно поменять оператор логического И на оператор логического ИЛИ либо удалить эту часть выражения.

Ещё несколько примеров ошибок в слишком длинных строках:

• V501 There are identical sub-expressions 'NPC[A].Type == 193' to the left and to the right of the '||' operator. thextech npc_update.cpp 996
• V501 There are identical sub-expressions 'NPC[A].Type == 193' to the left and to the right of the '||' operator. thextech npc_update.cpp 1033
• V501 There are identical sub-expressions 'NPC[A].Type != 191' to the left and to the right of the '&&' operator. thextech npc_update.cpp 2869
• V547 Expression 'NPC[A].Type == 54 && NPC[A].Type == 15' is always false. Probably the '||' operator should be used here. thextech npc_update.cpp 1277

Фрагмент N2

Следующий фрагмент кода уже был форматирован для чтения человеком. Впрочем, несмотря на больший шанс заметить здесь ошибки, они остались. Причём в количестве 4 штук:

• V501 There are identical sub-expressions 'n.Type == 159' to the left and to the right of the '||' operator. thextech menu_loop.cpp 324
• V501 There are identical sub-expressions 'n.Type == 160' to the left and to the right of the '||' operator. thextech menu_loop.cpp 324
• V501 There are identical sub-expressions 'n.Type == 164' to the left and to the right of the '||' operator. thextech menu_loop.cpp 324
• V501 There are identical sub-expressions 'n.Type == 197' to the left and to the right of the '||' operator. thextech menu_loop.cpp 324

Здесь нет смысла дважды проверять одни и те же значения и лишние сравнения можно удалить.

Далее можно обойтись без скриншотов.

Фрагмент N3

V501 There are identical sub-expressions '(evt.AutoSection) >= (0)' to the left and to the right of the '&&' operator. thextech layers.cpp 568

#define IF_INRANGE(x, l, r)  ((x) >= (l) && (x) <= (r))

else if(  IF_INRANGE(evt.AutoSection, 0, maxSections)
       && IF_INRANGE(evt.AutoSection, 0, maxEvents))
{
  // Buggy behavior, see https://github.com/Wohlstand/TheXTech/issues/44
  AutoX[evt.AutoSection] = Events[evt.AutoSection].AutoX;
  AutoY[evt.AutoSection] = Events[evt.AutoSection].AutoY;
}

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

((evt.AutoSection) >= (0) && (evt.AutoSection) <= (maxSections)) &&
((evt.AutoSection) >= (0) && (evt.AutoSection) <= (maxEvents))

Подобные срабатывания можно подавлять или переписать условие как-то так:

IF_INRANGE(evt.AutoSection, 0, min(maxSections, maxEvents))

Также на эту строку сработала диагностика V590.

V590 Consider inspecting this expression. The expression is excessive or contains a misprint. thextech layers.cpp 568

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

На мой взгляд, интересный момент можно найти в issues из ссылки в комментарии из примера. Там пользователь с ником ds-sloth предложил исправить строчку:

AutoX[Events[A].AutoSection] = Events[Events[A].AutoSection].AutoX;

на

AutoX[Events[A].AutoSection] = Events[A].AutoX;

Это исправление чинит управление автопрокруткой уровня через внутриигровые события:

Однако этот фикс по умолчанию остался в выключенном состоянии, поскольку меняет или ломает оригинальное поведение игры:

Wohlstand: I guess the fix of this bug doesn't break levels that intended to work it, but breaks level that contains an absolutely invalid data and didn't intended to auto-scroll at all (knowing it was broken), in the result the issue #65. I guess since the 38A's per-section auto-scroll setup was utilized, when I will implement the way at PGE Editor to set them, I'll set this autoscrolling compat.ini value into 'false' by default.

Wohlstand: Okay, I had to set the auto-scroll fix into 'false', mainly because there are several faulty levels that kept bad auto-scroll data after unsuccess experiments done by users, they making much more pain than proper use of this thing.

Поэтому в некоторых случаях исправление ошибок, которые показывает анализатор, требует дополнительного обдумывания, поскольку это может нарушить багосовместимость :), например, в следующих примерах.

Фрагмент N4

V501 There are identical sub-expressions to the left and to the right of the '!=' operator: NPC[A].Projectile != NPC[A].Projectile thextech npc_hit.cpp 2105

else if (  NPC[A].Location.SpeedX != oldNPC.Location.SpeedX
        || NPC[A].Location.SpeedY != oldNPC.Location.SpeedY
        || NPC[A].Projectile != NPC[A].Projectile  // <=
        || NPC[A].Killed != oldNPC.Killed
        || NPC[A].Type != oldNPC.Type
        || NPC[A].Inert != oldNPC.Inert)
{ .... }

В этом условии сравнивается набор полей между объектами NPC[A] и oldNPC. Посередине поле Projectile сравнивается у NPC[A] само с собой. Очень похоже на неаккуратный копипаст. Классика. Однако, к чему может привести исправление этого условия, можно узнать только тестированием или полным пониманием игровой логики. Возможно, там просто лишняя проверка.

Аналогичная ошибка:

• V501 There are identical sub-expressions to the left and to the right of the '!=' operator: NPC[A].Projectile != NPC[A].Projectile thextech npc_hit.cpp 2129

Фрагмент N5

И последняя на сегодня ошибка V501:

V501 There are identical sub-expressions 'MenuMode == MENU_SELECT_SLOT_1P_DELETE' to the left and to the right of the '||' operator. thextech menu_main.cpp 1004

// Delete gamesave
else if(  MenuMode == MENU_SELECT_SLOT_1P_DELETE
       || MenuMode == MENU_SELECT_SLOT_1P_DELETE)
{
  if(MenuMouseMove)
    s_handleMouseMove(2, 300, 350, 300, 30);
....

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

Чуть ниже в блоке этого условия есть такое же срабатывание:

• V501 There are identical sub-expressions 'MenuMode == MENU_SELECT_SLOT_1P_DELETE' to the left and to the right of the '||' operator. thextech menu_main.cpp 1004

Проблемы с условными операторами

Фрагмент N6

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

if(Player[A].Character == 2) // luigi doesn't fly as long as mario
  Player[A].FlyCount = 300; // Length of flight time
else if(Player[A].Character == 3) // special handling for peach
{
  Player[A].FlyCount = 0;
  Player[A].RunCount = 80;
  Player[A].CanFly2 = false;
  Player[A].Jump = 70;
  Player[A].CanFloat = true;
  Player[A].FlySparks = true;
}
else if(Player[A].Character == 3) // special handling for peach
  Player[A].FlyCount = 280; // Length of flight time
else
  Player[A].FlyCount = 320; // Length of flight time

В этом фрагменте последовательно выполняют проверки несколько конструкций else-if с одинаковым условием (Player[A].Character == 3). Это приводит к недостижимому коду во второй конструкции else-if. Похоже на то, что этот код не даёт принцессе Пич летать в каких-то случаях. Стоит попробовать убрать лишнюю ветку и просто присвоить число 280 в переменную Player[A].FlyCount.

Фрагмент N7

В следующем примере находится подозрительное дублирование кода в then и else ветках условия:

V523 The 'then' statement is equivalent to the 'else' statement. thextech npc_hit.cpp 1546

if(NPC[C].Projectile && !(NPC[C].Type >= 117 && NPC[C].Type <= 120))
{
  if(!(NPC[A].Type == 24 && NPC[C].Type == 13))
    NPC[A].Killed = B;
  else
    NPC[A].Killed = B;
}

Возможно, здесь не работает какое-то специальное исключение при определении, может ли снаряд убить NPC конкретного типа.

Фрагмент N8

В одном из срабатываний попалось невыполнимое условие:

V547 Expression 'A == 48' is always false. thextech effect.cpp 1652

else if(A == 16) // Dead Giant Bullet Bill
{
  numEffects++;
  Effect[numEffects].Shadow = Shadow;
  ....
  Effect[numEffects].Location.SpeedY = Location.SpeedY;
  Effect[numEffects].Location.SpeedX = Location.SpeedX;
  if(A == 48)                                          // <=
    Effect[numEffects].Location.SpeedY = -8;
  Effect[numEffects].Life = 120;
  Effect[numEffects].Type = A;
}

Поскольку в этот блок программа может зайти, только если переменная A равна 16, условие 'A == 48' не выполнится. В результате у эффекта будет установлена неправильная вертикальная скорость, и Гигантский Пуля Билл погибнет недостаточно драматично :)

Фрагмент N9

Ещё один пример бесполезного условного оператора:

V547 Expression 'tempPlayer == 0' is always true. thextech blocks.cpp 576

// don't spawn players from blocks anymore
tempPlayer = 0;
if(tempPlayer == 0) // Spawn the npc
{
  numNPCs++; // create a new NPC
  NPC[numNPCs].Active = true;
  NPC[numNPCs].TimeLeft = 1000;
....

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

Фрагмент N10

А здесь лишняя проверка на то, что логический результат сравнения не равен 0:

V562 It's odd to compare a bool type value with a value of 0. thextech editor.cpp 102

if(!MagicHand)
{
  if((getKeyState(vbKeyPageUp) == KEY_PRESSED) != 0)  // <=
  {
    if(ScrollRelease == true)
....

Можно написать просто:

if(getKeyState(vbKeyPageUp) == KEY_PRESSED)

Ещё такие же срабатывания:

• V562 It's odd to compare a bool type value with a value of 0. thextech editor.cpp 115
• V562 It's odd to compare a bool type value with a value of 0. thextech editor.cpp 170

Фрагмент N11

В следующем примере может содержаться логическая ошибка. В условии сначала проверяется значение в массиве по индексу whatPlayer и только после этого проверяется диапазон переменной whatPlayer:

V781 The value of the 'whatPlayer' index is checked after it was used. Perhaps there is a mistake in program logic. thextech blocks.cpp 159

if(b.ShakeY != 0 || b.ShakeY2 != 0 || b.ShakeY3 != 0)
{
  if(  b.RapidHit > 0
    && Player[whatPlayer].Character == 4 && whatPlayer > 0) // <=
  {
    b.RapidHit = (iRand() % 3) + 1;
  }
  return;
}

Это может привести к неопределённому поведению.

Фрагмент N12

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

V570 The 'NPC[A].Location.X' variable is assigned to itself. thextech npc_hit.cpp 1995

else
{
  NPC[A].Location.Y = NPC[A].Location.Y + NPC[A].Location.Height;
  NPC[A].Location.X = NPC[A].Location.X; // - (32 - .Location.Width) / 2
  ....
}

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

Ещё примеры лишнего присваивания:

• V570 The 'Player[A].MountOffsetY' variable is assigned to itself. thextech player.cpp 1861
• V570 The 'tempLocation.X' variable is assigned to itself. thextech npc_update.cpp 4177
• V570 The 'tempLocation.Width' variable is assigned to itself. thextech npc_update.cpp 4178

Прочие ошибки

Фрагмент N13

Странный цикл в функции, которая пытается прочитать JPEG изображение:

V654 The condition 'chunk_size > 0' of loop is always true. thextech image_size.cpp 211

static bool tryJPEG(SDL_RWops* file, uint32_t *w, uint32_t *h)
{
  ....
  size_t chunk_size = 0;
  ....
  do
  {
    SDL_memset(raw, 0, JPEG_BUFFER_SIZE);
    pos = SDL_RWtell(file);
    chunk_size = SDL_RWread(file, raw, 1, JPEG_BUFFER_SIZE);
    if(chunk_size == 0)
      break;

    head = findJpegHead(raw, JPEG_BUFFER_SIZE);
    if(head)
    {
      if(head + 20 >= raw + JPEG_BUFFER_SIZE)
      {
        SDL_RWseek(file, -20, RW_SEEK_CUR);
        continue; /* re-scan this place */
      }

      if(SDL_memcmp(head, "\xFF\xE1", 2) == 0) /* EXIF, skip it!*/
      {
        const Sint64 curPos = pos + (head - raw);
        Sint64 toSkip = BE16(head, 2); //-V629
        SDL_RWseek(file, curPos + toSkip + 2, RW_SEEK_SET);
        continue;
      }

      *h = BE16(head, 5);
      *w = BE16(head, 7);
      return true;
    }
  } while(chunk_size > 0);               // <=

  return false;
}

Переменная chunk_size обновляется практически в самом начале итерации цикла и, если она оказывается равной 0, цикл завершается. После этого она дойдёт до проверки условия выхода из цикла, при этом гарантированно будет больше нуля. Здесь можно просто использовать бесконечный цикл 'while (true)'.

Фрагмент N14

В этом выражении использовали оператор побитового ИЛИ вместо логического между вызовами функций, возвращающих bool. В результате всегда будут выполнены обе функции, что менее эффективно:

V792 The 'vScreenCollision' function located to the right of the operator '|' will be called regardless of the value of the left operand. Perhaps, it is better to use '||'. thextech gfx_update.cpp 1007

bool vScreenCollision(int A, const Location_t &Loc2)
....

// warp NPCs
if(Player[A].HoldingNPC > 0 && Player[A].Frame != 15)
{
  if((  vScreenCollision(Z, NPC[Player[A].HoldingNPC].Location)
      | vScreenCollision(Z, newLoc(....))) != 0       // <=
    && NPC[Player[A].HoldingNPC].Hidden == false)
  {
....

Такая же ошибка встречается ещё в нескольких местах:

• V792 The 'vScreenCollision' function located to the right of the operator '|' will be called regardless of the value of the left operand. Perhaps, it is better to use '||'. thextech gfx_update.cpp 1253
• V792 The 'vScreenCollision' function located to the right of the operator '|' will be called regardless of the value of the left operand. Perhaps, it is better to use '||'. thextech gfx_update.cpp 1351
• V792 The 'vScreenCollision' function located to the right of the operator '|' will be called regardless of the value of the left operand. Perhaps, it is better to use '||'. thextech gfx_update.cpp 1405
• V792 The 'CheckCollision' function located to the right of the operator '|' will be called regardless of the value of the left operand. Perhaps, it is better to use '||'. thextech player.cpp 4172

Фрагмент N15

В следующем примере зачем-то лишний раз конструируют строку, передавая результат вызова метода c_str() в функцию, которая принимает ссылку на std::string. Такой код менее эффективен, поскольку при преобразовании из std::string в char* теряется информация о текущей длине строки. И при последующем конструировании нового std::string длину придётся пересчитать линейным поиском терминального нуля. Причём этот момент компилятором не оптимизируется (проверено через Clang с -O3 оптимизациями).

V811 Decreased performance. Excessive type casting: string -> char * -> string. Consider inspecting first argument of the function open_file. thextech graphics_funcs.cpp 63

bool FileMapper::open_file(const std::string& path)
{
  return d->openFile(path);
}

FIBITMAP *GraphicsHelps::loadImage(std::string file, bool convertTo32bit)
{
....
  if(!fileMap.open_file(file.c_str())) // <=
        return nullptr;
....
}

Фрагмент N16

В этом цикле многократно вызывается расчёт длины одних и тех же строк. Стоит объявить их как константы типа std::string и пользоваться методом size():

V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. thextech menu_main.cpp 1027

#define For(A, From, To) for(int A = From; A <= To; ++A)

if(MenuMouseMove)
{
  For(A, 0, optionsMenuLength)
  {
    if(MenuMouseY >= 350 + A * 30 && MenuMouseY <= 366 + A * 30)
    {
      if(A == 0)
        menuLen = 18 * std::strlen("player 1 controls") - 4; // <=
      else if(A == 1)
        menuLen = 18 * std::strlen("player 2 controls") - 4; // <=
....

И такой паттерн встречается достаточно часто:

• V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. thextech menu_main.cpp 1029
• V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. thextech menu_main.cpp 1034
• V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. thextech menu_main.cpp 1036
• V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. thextech menu_main.cpp 1040
• V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. thextech menu_main.cpp 1131
• V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. thextech menu_main.cpp 1174
• V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. thextech menu_main.cpp 1200
• V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. thextech menu_main.cpp 1204

Заключение

Если верить Википедии, первый публичный релиз TheXTech был выпущен всего через месяц после публикации оригинальных исходников SMBX. Это, на мой взгляд, очень круто для полноценного кроссплатформенного перевода проекта на другой язык программирования (особенно на C++).

В случае если планируется капитальная переработка кода, возможно, стоит начать использовать PVS-Studio, ведь мы даём бесплатную лицензию для open-source проектов.

В качестве бонуса – видео с нашего YouTube канала на тематику Марио:

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Alexey Govorov. Thanks, Mario, but the code needs fixing — checking TheXTech.