Спасибо, Марио, но код нуждается в исправлении

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

О проекте

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

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

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

Очистка кода

Фрагмент 1

Видите, какую ошибку обнаружил анализатор ниже?

V547 Выражение NPC[A].Type == 54 && NPC[A].Type == 15 всегда ложно. Вероятно, здесь следует использовать оператор ||. 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 Слева и справа от оператора || одинаковые подвыражения NPC[A].Type == 193. npc_update.cpp 996
V501 Слева и справа от оператора «||» есть одинаковые подвыражения «NPC[A].Type == 193». npc_update.cpp 1033
V501 Слева и справа от оператора && находятся одинаковые подвыражения NPC[A].Type != 191. npc_update.cpp 2869
V547 Выражение NPC[A].Type == 54 && NPC[A].Type == 15 всегда ложно. Вероятно, здесь следует использовать оператор ||. npc_update.cpp 1277

Фрагмент 2

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

V501 Слева и справа от оператора || одинаковые подвыражения n.Type == 159. thextech menu_loop.cpp 324
V501 Слева и справа от оператора «||» есть одинаковые подвыражения ‘n.Type == 160’. thextech menu_loop.cpp 324
V501 Слева и справа от оператора «||» есть одинаковые подвыражения ‘n.Type == 164’. thextech menu_loop.cpp 324
V501 Слева и справа от оператора «||» есть одинаковые подвыражения ‘n.Type == 197’. thextech menu_loop.cpp 324

Вы можете нажать на картинку, чтобы увидеть выделенные ошибки.

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

Скриншоты дальше не нужны.

Фрагмент 3

V501 Слева и справа от оператора && одинаковые подвыражения (evt.AutoSection) ›= (0). слои текстеха.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 Подумайте о проверке этого выражения. Выражение является избыточным или содержит опечатку. слои текстеха.cpp 568

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

Кстати, в этом фрагменте кода можно найти интересный момент. Просто перейдите по ссылке из комментария к фрагменту кода и посмотрите проблему. Пользователь ds-sloth предложил следующее исправление — изменить эту строку:

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

в это:

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

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

Вы можете нажать на картинку, чтобы увидеть анимацию.

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

Wohlstand: Я предполагаю, что исправление этой ошибки не ломает уровни, предназначенные для ее работы, но ломает уровень, который содержит абсолютно неверные данные и вообще не предназначен для автоматической прокрутки (зная это был сломан), в результате чего вышел выпуск №65. Я предполагаю, что, поскольку в 38A была использована настройка автоматической прокрутки для каждого раздела, когда я реализую способ их установки в редакторе PGE, я установлю это значение autoscrolling compat.ini по умолчанию на «false».

Вольстанд: Хорошо, мне пришлось установить исправление автоматической прокрутки на «false», в основном потому, что есть несколько ошибочных уровней, которые сохраняли неверные данные автоматической прокрутки после неудачных экспериментов, проведенных пользователями, они делают гораздо больше боль, чем правильное использование этой вещи.

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

Фрагмент четвертый

V501 Слева и справа от оператора != одинаковые подвыражения: 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 Слева и справа от оператора ‘!=’ есть одинаковые подвыражения: NPC[A].Projectile != NPC[A].Projectile thextech npc_hit.cpp 2129

Фрагмент 5

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

V501 Слева и справа от оператора || одинаковые подвыражения MenuMode == MENU_SELECT_SLOT_1P_DELETE. текстех 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 Слева и справа от оператора «||» есть идентичные подвыражения «MenuMode == MENU_SELECT_SLOT_1P_DELETE». текстех menu_main.cpp 1004

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

Фрагмент шесть

V517 Обнаружено использование шаблона if (A) {…} else if (A) {…}. Существует вероятность наличия логической ошибки. Контрольные строки: 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.

Фрагмент седьмой

Анализатор обнаружил подозрительное дублирование кода в ветках then и else условия:

V523 Оператор тогда эквивалентен оператору еще. 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.

Фрагмент восемь

Анализатор обнаружил невозможное условие:

V547 Выражение А == 48 всегда ложно. Эффект thextech.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 никогда не выполняется. В результате эффект будет иметь неправильную вертикальную скорость. Так что смерть Гигантского Пуля Билла будет недостаточно драматичной. :)

Фрагмент девять

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

V547 Выражение tempPlayer == 0 всегда верно. 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 всегда инициализируется нулем. Мы можем уменьшить вложенность кода, удалив ненужное условие.

Фрагмент десять

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

V562 Странно сравнивать значение типа bool со значением 0. thextech editor.cpp 102

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

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

if(getKeyState(vbKeyPageUp) == KEY_PRESSED)

Еще такие предупреждения:

V562 Странно сравнивать значение типа bool со значением 0. thextech editor.cpp 115
V562 Странно сравнивать значение типа bool со значением 0. thextech editor.cpp 170

Фрагмент одиннадцать

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

V781 Значение индекса whatPlayer проверяется после его использования. Возможно, ошибка в логике программы. 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;
}

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

Фрагмент двенадцать

Немного странный фрагмент. После того, как разработчик закомментировал часть выражения, переменная стала присваивать себе одно и то же значение:

V570 Переменная NPC[A].Location.X присваивается самой себе. 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 Переменная Player[A].MountOffsetY присваивается самой себе. thextech player.cpp 1861
V570 Переменная ‘tempLocation.X’ присваивается самой себе. npc_update.cpp 4177
V570 Переменная ‘tempLocation.Width’ присваивается самой себе. npc_update.cpp 4178

Другие ошибки

Фрагмент тринадцать

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

V654 Условие ‘chunk_size › 0’ цикла всегда верно. xtech 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 обновляется почти в самом начале итерации цикла. Если переменная равна нулю, цикл прерывается. После этого переменная переходит на проверку условия выхода из цикла. Однако гарантировано, что оно больше нуля. Здесь мы можем использовать бесконечный цикл while (true).

Фрагмент четырнадцать

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

V792 Функция vScreenCollision, расположенная справа от оператора |, будет вызываться вне зависимости от значения левого операнда. Возможно, лучше использовать ‘||’. 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 Функция «vScreenCollision», расположенная справа от оператора «|», будет вызываться независимо от значения левого операнда. Возможно, лучше использовать ‘||’. thextech gfx_update.cpp 1253
V792 Функция «vScreenCollision», расположенная справа от оператора «|», будет вызываться независимо от значения левого операнда. Возможно, лучше использовать ‘||’. thextech gfx_update.cpp 1351
V792 Функция «vScreenCollision», расположенная справа от оператора «|», будет вызываться независимо от значения левого операнда. Возможно, лучше использовать ‘||’. thextech gfx_update.cpp 1405
V792 Функция «CheckCollision», расположенная справа от оператора «|», будет вызываться независимо от значения левого операнда. Возможно, лучше использовать ‘||’. thextech player.cpp 4172

Фрагмент пятнадцать

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

V811 Снижение производительности. Чрезмерное приведение типов: string -> char * -> string. Рассмотрите возможность проверки первого аргумента функции open_file. xtech 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;
....
}

Фрагмент шестнадцатый

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

V814 Снижение производительности. Функция strlen вызывалась несколько раз внутри тела цикла. текстех 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 Снижение производительности. Функция strlen вызывалась несколько раз внутри тела цикла. текстех menu_main.cpp 1029
V814 Снижение производительности. Функция strlen вызывалась несколько раз внутри тела цикла. текстех menu_main.cpp 1034
V814 Снижение производительности. Функция strlen вызывалась несколько раз внутри тела цикла. текстех menu_main.cpp 1036
V814 Снижение производительности. Функция strlen вызывалась несколько раз внутри тела цикла. текстех menu_main.cpp 1040
V814 Снижение производительности. Функция strlen вызывалась несколько раз внутри тела цикла. текстех menu_main.cpp 1131
V814 Снижение производительности. Функция strlen вызывалась несколько раз внутри тела цикла. текстех menu_main.cpp 1174
V814 Снижение производительности. Функция strlen вызывалась несколько раз внутри тела цикла. текстех menu_main.cpp 1200
V814 Снижение производительности. Функция strlen вызывалась несколько раз внутри тела цикла. текстех menu_main.cpp 1204

Заключение

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

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

Спасибо, Марио, но код нуждается в исправлении — проверка TheXTech