Thanks, Mario, but the code needs fixing — checking TheXTech

About the project

TheXTech is the SMBX 1.3. game engine rewritten on C++. The original SMBX (Super Mario Bros. X) was written on Visual Basic 6 by Andrew Spinks in 2009. It allows to create levels from the elements of the Nintendo’s Super Mario Bros games. TheXTech accurately reproduces the original game’s behavior. It also includes optional bug fixes. It runs not only on Windows, but also on macOS, and Linux systems with x86, ARM or PowerPC processors. Some developers also ported it on 3DS and PS Vista

Results of the check

Cleaning the code

Fragment one

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)) // <=
|| .... )
{ .... }
  • 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
  • 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
#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))
AutoX[Events[A].AutoSection] = Events[Events[A].AutoSection].AutoX;
AutoX[Events[A].AutoSection] = Events[A].AutoX;
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)
{ .... }
// 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);
....

Problems with conditional operators

Fragment six

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
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;
}
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;
}
// 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;
....
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 170
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;
}
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 ‘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

Other errors

Fragment thirteen

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;
}
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 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
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;
....
}
#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 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

Conclusion

According to Wikipedia (ru), TheXTech was first publicly released just a month after the SMBX source code was published. It’s really cool for a full cross-platform project porting to another language. Especially on C++.

--

--

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store
Unicorn Developer

Unicorn Developer

609 Followers

The developer, the debugger, the unicorn. I know all about static analysis and how to find bugs and errors in C, C++, C#, and Java source code.