# Thanks, Mario, but the code needs fixing — checking TheXTech It's cool when enthusiastic developers create a working clone of a famous game\. It's even cooler when people are ready to continue the development of such projects\! In this article, we check TheXTech with PVS\-Studio\. TheXTech is an open implementation of the game from the Super Mario universe\. ![0889_TheXTech/image1.png](https://import.viva64.com/docx/blog/0889_TheXTech/image1.png) ## About the project [TheXTech](https://github.com/Wohlstand/TheXTech) is the SMBX 1\.3\. game engine rewritten on C\+\+\. The original [SMBX](https://github.com/smbx/smbx-legacy-source) \(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 The TheXTech developer — Vitaliy Novichkov \(Wohlstand\) — described the development process in detail on [Habr](https://bit.ly/32ny9nn)\. He also described the techniques he used to smooth out the differences when porting the project from VB6 to C\+\+\. There's a [disclaimer](https://github.com/Wohlstand/TheXTech/blob/d07a7ce1c9582084cf118d185f41a1eafef9e4cd/README.md) on the GitHub page that explains why the source code is not in the best condition\. It's because the original code is unstructured something fierce\. Its fragments you'll see below\. ## Results of the check ### Cleaning the code **Fragment one** Can you see the error the analyzer found below? [V547](https://pvs-studio.com/en/docs/warnings/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 ![0889_TheXTech/image3.png](https://import.viva64.com/docx/blog/0889_TheXTech/image3.png) Of course not :\) The error hides in the middle of the condition in string that has a 1400 character long\. You have to scroll 5 screens to the right to find it\. Let's format the code: ```cpp 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)) // <= || .... ) { .... } ``` Now you can see it\. _The NPC\[A\]\.Type_variable cannot be equal to two different values at the same time\. Apparently, the condition was intended to be true for projectiles of types 54 and 15\. However, now this part of the condition is always false\. The developer should have changed the _AND_ logical operator to the _OR_ logical operator\. Another option is to delete this part of the expression\. A couple of error examples in too long lines: * [V501](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/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 **Fragment two** The next code fragment was formatted for reading\. Despite the greater chance of noticing errors here, someone missed them\. Even 4 of them: * [V501](https://pvs-studio.com/en/docs/warnings/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 ![0889_TheXTech/image5.png](https://import.viva64.com/docx/blog/0889_TheXTech/image5.png) You can click on the picture to see the highlighted errors\. There is no point in double checking the same values here\. Unnecessary comparisons can be removed\. No screenshots needed further\. **Fragment three** [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions '\(evt\.AutoSection\) \>\= \(0\)' to the left and to the right of the '&&' operator\. thextech layers\.cpp 568 ```cpp #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; } ``` In this code fragment the analyzer was confused by the duplication of expressions\. This duplication appeared as a result of the macro expansion: ```cpp ((evt.AutoSection) >= (0) && (evt.AutoSection) <= (maxSections)) && ((evt.AutoSection) >= (0) && (evt.AutoSection) <= (maxEvents)) ``` Such warnings can be suppressed\. The developer can also rewrite the condition like this: ```cpp IF_INRANGE(evt.AutoSection, 0, min(maxSections, maxEvents)) ``` This string also triggered the V590 rule\. [V590](https://pvs-studio.com/en/docs/warnings/v590/) Consider inspecting this expression\. The expression is excessive or contains a misprint\. thextech layers\.cpp 568 If we fix those warnings, it won't fix any bugs\. The compilers delete unnecessary constructions anyway\. However, we can clean up the code this way\. By the way, you can find an interesting moment, in this code fragment\. Just follow the link from the code fragment's comment and look at the [issue](https://github.com/Wohlstand/TheXTech/issues/44)\. A user named ds\-sloth suggested the following fix — to change this line: ```cpp AutoX[Events[A].AutoSection] = Events[Events[A].AutoSection].AutoX; ``` into this: ```cpp AutoX[Events[A].AutoSection] = Events[A].AutoX; ``` This change would fix the auto\-scroll mechanism that is controlled by in\-game events: ![](https://import.viva64.com/docx/blog/0889_TheXTech/image6.mp4) You can click on the picture to see the animation\. However, this fix is disabled by default because it changes or breaks the game behavior: > **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\. Therefore, in some cases, fixing the error requires consideration — fixing some of them may break the [bug compatability](https://en.wikipedia.org/wiki/Bug_compatibility) :\)\. The following examples show such cases\. **Fragment four** [V501](https://pvs-studio.com/en/docs/warnings/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 ```cpp 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) { .... } ``` This code fragment compares a set of data members in the _NPC\[A\]_ and _oldNPC_ objects\. In the middle of this fragment the _Projectile_ members of _NPC\[A\]_ is compared with itself\. Looks like a sloppy copypaste\. Classic\. However, only testing \(or a full understanding of the game's logic\) shows what would happen after we fix this condition\. Maybe there's just an redundant check\. Similar error: * 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 **Fragment five** The last V501 error for today: [V501](https://pvs-studio.com/en/docs/warnings/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 ```cpp // 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); .... ``` It's unclear whether only the first player should have the right to delete the save slot\. In this case, the additional check for MENU\_SELECT\_SLOT\_1P\_DELETE is unnecessary here\. Nevertheless, the code has the MENU\_SELECT\_SLOT\_2P\_DELETE constant\. Probably, this constant should have been used in the right part of the expression\. This condition block has the same warning just below: * 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 ### Problems with conditional operators **Fragment six** [V517](https://pvs-studio.com/en/docs/warnings/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 ```cpp 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 ``` In this fragment, several _else\-if_ constructs with the same condition \(_Player\[A\]\.Character \=\= 3_\) do subsequent checks\. This leads to the unreachable code in the second _else\-if_ construct\. Looks like this code fragment prevents Princess Peach from flying in some places\. We can try to remove the extra branch and simply assign 280 to the _Player\[A\]\.FlyCount_ variable\. **Fragment seven** The analyzer has detected suspicious code duplication in the _then_ and _else_ branches of condition: [V523](https://pvs-studio.com/en/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. thextech npc\_hit\.cpp 1546 ```cpp 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; } ``` Maybe some special exception is broken where this function determines whether a projectile can kill a specific type of NPC\. **Fragment eight** The analyzer has detected an impossible condition: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'A \=\= 48' is always false\. thextech effect\.cpp 1652 ```cpp 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; } ``` Since the program can enter this block only if the _A_ variable equals 16, the _A \=\= 48_ condition is never fulfilled\. As a result the effect will have the wrong vertical velocity\. So the Giant Bullet Bill's death will not be dramatic enough\. :\) **Fragment nine** Another example of a useless conditional operator: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'tempPlayer \=\= 0' is always true\. thextech blocks\.cpp 576 ```cpp // 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; .... ``` Apparently, after refactoring, the _tempPlayer_ variable is always initialized to zero\. We can reduce the code nesting by removing an unnecessary condition\. **Fragment ten** Here is an extra check that the logical result of the comparison is not equal to 0: [V562](https://pvs-studio.com/en/docs/warnings/v562/) It's odd to compare a bool type value with a value of 0\. thextech editor\.cpp 102 ```cpp if(!MagicHand) { if((getKeyState(vbKeyPageUp) == KEY_PRESSED) != 0) // <= { if(ScrollRelease == true) .... ``` We can write simply: ```cpp if(getKeyState(vbKeyPageUp) == KEY_PRESSED) ``` More of such warnings: * 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 **Fragment eleven** The following example may contain a logical error\. The condition first checks the value of the array by the _whatPlayer_ index\. Only after that the fragment checks the _whatPlayer_ variable's range: [V781](https://pvs-studio.com/en/docs/warnings/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 ```cpp 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; } ``` This may result in undefined behavior\. **Fragment twelve** A bit weird fragment\. After the developer commented the part of an expression, the variable began to assign itself the same value: [V570](https://pvs-studio.com/en/docs/warnings/v570/) The 'NPC\[A\]\.Location\.X' variable is assigned to itself\. thextech npc\_hit\.cpp 1995 ```cpp 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 .... } ``` The program's behavior doesn't change from such expressions\. However, this code fragment may indicate logical errors\. For example, a logical error appears if, after debugging, the developer doesn't put the commented fragment back\. There are examples of unnecessary assignment: * 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 ### Other errors **Fragment thirteen** A weird loop in a function that tries to read a JPEG image: [V654](https://pvs-studio.com/en/docs/warnings/v654/) The condition 'chunk\_size \> 0' of loop is always true\. thextech image\_size\.cpp 211 ```cpp 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; } ``` The _chunk\_size_ variable is updated almost at the very beginning of the loop iteration\. If the variable equals zero, the loop breaks\. After that the variable goes to checking the exit condition of the loop\. However, it is guaranteed to be greater than zero\. Here we can use the infinite _while \(true\)_ loop\. **Fragment fourteen** This code fragment has the bitwise _OR_ operator instead of the logical one\. This operator is used between calls of functions that return _bool_\. As a result, both functions are always executed, which is less effective: [V792](https://pvs-studio.com/en/docs/warnings/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 ```cpp 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) { .... ``` The same error appears in other places: * 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 **Fragment fifteen** In the following example the developer constructs an unnecessary string, passing the result of calling the _c\_str\(\)_ member function\. The developer passes it to the function which accepts a reference to _std::string_\. The code is less efficient that way\. When the developer converts _std::string_ into _char\*_, information about the current length of the string is lost\. When subsequently constructing new _std::string_, the program has to recalculate the length by a linear search for terminal null character\. The compiler doesn't optimize this moment — we checked it with [Clang](https://godbolt.org/z/E9Mq6197d) with \-O3 optimizations\. [V811](https://pvs-studio.com/en/docs/warnings/v811/) Decreased performance\. Excessive type casting: string \-\> char \* \-\> string\. Consider inspecting first argument of the function open\_file\. thextech graphics\_funcs\.cpp 63 ```cpp 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; .... } ``` **Fragment sixteen** In this loop the length of the same strings is repeatedly calculated\. The developer should declare it as constants of the _std::string_ type and use the _size\(\)_ method: [V814](https://pvs-studio.com/en/docs/warnings/v814/) Decreased performance\. The 'strlen' function was called multiple times inside the body of a loop\. thextech menu\_main\.cpp 1027 ```cpp #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; // <= .... ``` This pattern is quite common: * 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 ## Conclusion According to [Wikipedia \(ru\)](https://bit.ly/30of57X), 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\+\+\. Developers planning a major code revision can try PVS\-Studio\. We provide a [free](https://pvs-studio.com/en/order/open-source-license/) license for open source projects\. As a bonus — here's the Mario\-themed video from our YouTube channel: