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

Давайте немного поностальгируем и вспомним любимую многими игру - Super Mario. Это серия компьютерных игр в жанре платформер, издаваемых компанией Nintendo. Часть медиафраншизы Mario. Первая игра в серии — Super Mario Bros. — вышла аж в 1985 году.

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

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

Полный список найденных фрагментов вы можете почитать здесь

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

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

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

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

О проекте

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

Чистка кода

Заключение