# Anomalies in X\-Ray Engine The X\-Ray Engine is a game engine, used in the S\.T\.A\.L\.K\.E\.R\. game series\. Its code was made public in September 16 2014, and since then, STALKER fans continue its development\. A large project size, and a huge number of bugs in the games, gives us a wonderful chance to show what PVS\-Studio is capable of\. ![0405_XRay/image1.png](https://import.viva64.com/docx/blog/0405_XRay/image1.png) ## Introduction X\-Ray was created by a Ukrainian company, GSC GameWorld, for the game S\.T\.A\.L\.K\.E\.R\.: Shadow of Chernobyl\. This engine has a renderer supporting DirectX 8\.1/9\.0c/10/10\.1/11, physical and sound engines, multiplayer, and an artificial intelligence system \- A\-Life\. Later, the company was about to create a 2\.0 version for their new game, but development was discontinued, and the source code was put out for public access\. This project is easily built with all of its dependencies in Visual Studio 2015\. To do the analysis we used the engine source code 1\.6v, from a [repository on GitHub](https://github.com/openxray/xray-16), and PVS\-Studio 6\.05 static code analyze, which can be downloaded from this [link](https://pvs-studio.com/en/pvs-studio/download/)\. ## Copy\-paste Let's start with errors related to copying code\. The way they get to the code is usually the same: the code was copied, parts of variables were changed, and some remained forgotten\. Such errors can quickly spread in the code base, and are very easy to overlook without a static code analyzer\. ![0405_XRay/image2.png](https://import.viva64.com/docx/blog/0405_XRay/image2.png) ```cpp MxMatrix& MxQuadric::homogeneous(MxMatrix& H) const { .... unsigned int i, j; for(i=0; iline_exist(ppi_section,"color_base")) sscanf(ini->r_string(ppi_section,"color_base"), "%f,%f,%f", &data.m_attack_effector.ppi.color_base.r, &data.m_attack_effector.ppi.color_base.g, &data.m_attack_effector.ppi.color_base.b); if (ini->line_exist(ppi_section,"color_base")) sscanf(ini->r_string(ppi_section,"color_gray"), "%f,%f,%f", &data.m_attack_effector.ppi.color_gray.r, &data.m_attack_effector.ppi.color_gray.g, &data.m_attack_effector.ppi.color_gray.b); if (ini->line_exist(ppi_section,"color_base")) sscanf(ini->r_string(ppi_section,"color_add"), "%f,%f,%f", &data.m_attack_effector.ppi.color_add.r, &data.m_attack_effector.ppi.color_add.g, &data.m_attack_effector.ppi.color_add.b); .... } ``` **PVS\-Studio warnings:** * [V581](https://pvs-studio.com/en/docs/warnings/v581/) The conditional expressions of the 'if' operators situated alongside each other are identical\. Check lines: 445, 447\. base\_monster\_startup\.cpp 447 * [V581](https://pvs-studio.com/en/docs/warnings/v581/) The conditional expressions of the 'if' operators situated alongside each other are identical\. Check lines: 447, 449\. base\_monster\_startup\.cpp 449 In this fragment we see several conditional expressions in a row\. Obviously, we need to replace the _color\_base_ with _color\_gray_ _and_ _color\_add _according to the code in the _if_ branch\. ```cpp /* process a single statement */ static void ProcessStatement(char *buff, int len) { .... if (strncmp(buff,"\\pauthr\\",8) == 0) { ProcessPlayerAuth(buff, len); } else if (strncmp(buff,"\\getpidr\\",9) == 0) { ProcessGetPid(buff, len); } else if (strncmp(buff,"\\getpidr\\",9) == 0) { ProcessGetPid(buff, len); } else if (strncmp(buff,"\\getpdr\\",8) == 0) { ProcessGetData(buff, len); } else if (strncmp(buff,"\\setpdr\\",8) == 0) { ProcessSetData(buff, len); } } ``` **PVS\-Studio warning:** [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: 1502, 1505\. gstats\.c 1502 As in the previous example, two similar conditions are used here \(_strncmp\(buff,"\\\\getpidr\\\\",9\) \=\= 0_\)\. It's hard to say for sure whether this is a mistake, or simply unreachable code, but it's worth revising for sure\. Perhaps we should have blocks with _getpidr_/_setpidr _by analogy with _getpdr_/_setpdr_\. ```cpp class RGBAMipMappedCubeMap { .... size_t height() const { return cubeFaces[0].height(); } size_t width() const { return cubeFaces[0].height(); } .... }; ``` **PVS\-Studio warning:** [V524](https://pvs-studio.com/en/docs/warnings/v524/) It is odd that the body of 'width' function is fully equivalent to the body of 'height' function\. tpixel\.h 1090 Methods _height\(\)_ and _width\(\)_ have the same body\. Bearing in mind that we evaluate faces of a cube here, perhaps there is no error\. But it's better to rewrite the method _width\(\)_ in the following way: ```cpp size_t width() const { return cubeFaces[0].width(); } ``` ## Improper use of C\+\+ C\+\+ is a wonderful language that provides the programmer with many a possibility\.\.\. to shoot yourself in the foot in a multitude of the cruelest ways\. Undefined behavior, memory leaks, and of course, typos\. And that's what will be discussed in this section\. ![0405_XRay/image3.png](https://import.viva64.com/docx/blog/0405_XRay/image3.png) ```cpp template struct _matrix33 { public: typedef _matrix33Self; typedef Self& SelfRef; .... IC SelfRef sMTxV(Tvector& R, float s1, const Tvector& V1) const { R.x = s1*(m[0][0] * V1.x + m[1][0] * V1.y + m[2][0] * V1.z); R.y = s1*(m[0][1] * V1.x + m[1][1] * V1.y + m[2][1] * V1.z); R.z = s1*(m[0][2] * V1.x + m[1][2] * V1.y + m[2][2] * V1.z); } .... } ``` **PVS\-Studio warning:** [V591](https://pvs-studio.com/en/docs/warnings/v591/) Non\-void function should return a value\. \_matrix33\.h 435 At the end of the method there is no _return \*this\._ According to the standard, it will lead to undefined behavior\. As the return value is a reference, it will probably lead to a program crash, upon attempting to access the return value\. ```cpp ETOOLS_API int __stdcall ogg_enc(....) { .... FILE *in, *out = NULL; .... input_format *format; .... in = fopen(in_fn, "rb"); if(in == NULL) return 0; format = open_audio_file(in, &enc_opts); if(!format){ fclose(in); return 0; }; out = fopen(out_fn, "wb"); if(out == NULL){ fclose(out); return 0; } .... } ``` **PVS\-Studio warning:** [V575](https://pvs-studio.com/en/docs/warnings/v575/) The null pointer is passed into 'fclose' function\. Inspect the first argument\. ogg\_enc\.cpp 47 Quite an interesting example\. The analyzer detected that the argument in the _fclose _is _nullptr_, which makes the function call meaningless\. Presumably, the stream _in_ was to be closed\. ```cpp void NVI_Image::ABGR8_To_ARGB8() { // swaps RGB for all pixels assert(IsDataValid()); assert(GetBytesPerPixel() == 4); UINT hxw = GetNumPixels(); for (UINT i = 0; i < hxw; i++) { DWORD col; GetPixel_ARGB8(&col, i); DWORD a = (col >> 24) && 0x000000FF; DWORD b = (col >> 16) && 0x000000FF; DWORD g = (col >> 8) && 0x000000FF; DWORD r = (col >> 0) && 0x000000FF; col = (a << 24) | (r << 16) | (g << 8) | b; SetPixel_ARGB8(i, col); } } ``` **PVS\-Studio warnings:** * [V560](https://pvs-studio.com/en/docs/warnings/v560/) A part of conditional expression is always true: 0x000000FF\. nvi\_image\.cpp 170 * [V560](https://pvs-studio.com/en/docs/warnings/v560/) A part of conditional expression is always true: 0x000000FF\. nvi\_image\.cpp 171 * [V560](https://pvs-studio.com/en/docs/warnings/v560/) A part of conditional expression is always true: 0x000000FF\. nvi\_image\.cpp 172 * [V560](https://pvs-studio.com/en/docs/warnings/v560/) A part of conditional expression is always true: 0x000000FF\. nvi\_image\.cpp 173 In this fragment, we see that logical and bitwise operations get confused\. The result will not be what the programmer expected: _col_ will be always 0x01010101 regardless of the input data\. Correct variant: ```cpp DWORD a = (col >> 24) & 0x000000FF; DWORD b = (col >> 16) & 0x000000FF; DWORD g = (col >> 8) & 0x000000FF; DWORD r = (col >> 0) & 0x000000FF; ``` Another example of strange code: ```cpp VertexCache::VertexCache() { VertexCache(16); } ``` **PVS\-Studio warning:** [V603](https://pvs-studio.com/en/docs/warnings/v603/) The object was created but it is not being used\. If you wish to call constructor, 'this\-\>VertexCache::VertexCache\(\.\.\.\.\)' should be used\. vertexcache\.cpp 6 Instead of calling a constructor from another, a new object of _VertexCache_ gets created, and then destroyed, to initialize the instance\. As a result, the members of the created object remain uninitialized\. ```cpp BOOL CActor::net_Spawn(CSE_Abstract* DC) { .... m_States.empty(); .... } ``` **PVS\-Studio warning:** [V530](https://pvs-studio.com/en/docs/warnings/v530/) The return value of function 'empty' is required to be utilized\. actor\_network\.cpp 657 The analyzer warns that the value returned by the function is not used\. It seems that the programmer confused the methods _empty\(\) _and _clear\(\)_: the _empty\(\)_ does not clear the array, but checks whether it is empty or not\. Such errors are quite [common](https://pvs-studio.com/en/blog/examples/v530/) in various projects\. The thing is that the name _empty\(\) _is not very obvious: some view it as an action \- deletion_\. _To avoid such ambiguity, it's a good idea to add_ has, _or_ is _to the beginning of the method: it would be harder to confuse _isEmpty\(\)_ with _clear\(\)\._ A similar warning: V530 The return value of function 'unique' is required to be utilized\. uidragdroplistex\.cpp 780 ```cpp size_t xrDebug::BuildStackTrace(EXCEPTION_POINTERS* exPtrs, char *buffer, size_t capacity, size_t lineCapacity) { memset(buffer, capacity*lineCapacity, 0); .... } ``` **PVS\-Studio warning:** [V575](https://pvs-studio.com/en/docs/warnings/v575/) The 'memset' function processes '0' elements\. Inspect the third argument\. xrdebug\.cpp 104 During the _memset_ call the arguments got mixed up, and as a result the buffer isn't set to zero, as it was originally intended\. This error can live in a project for quite a long time, because it is very difficult to detect\. In such cases a static analyzer is of great help\. The correct use of _memset_: ```cpp memset(buffer, 0, capacity*lineCapacity); ``` The following error is connected with incorrectly formed logical expression\. ```cpp void configs_dumper::dumper_thread(void* my_ptr) { .... DWORD wait_result = WaitForSingleObject( this_ptr->m_make_start_event, INFINITE); while ( wait_result != WAIT_ABANDONED) || (wait_result != WAIT_FAILED)) .... } ``` **PVS\-Studio warning:** [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression is always true\. Probably the '&&' operator should be used here\. configs\_dumper\.cpp 262 The expression _x \!\= a \|\| x \!\= b_ is always true\. Most likely, && was meant to be here instead of \|\| operator\. More details on the topic of errors in logical expressions can be found in the article "[Logical Expressions in C/C\+\+\. Mistakes Made by Professionals](https://pvs-studio.com/en/blog/posts/cpp/0390/)"\. ```cpp void SBoneProtections::reload(const shared_str& bone_sect, IKinematics* kinematics) { .... CInifile::Sect &protections = pSettings->r_section(bone_sect); for (CInifile::SectCIt i=protections.Data.begin(); protections.Data.end() != i; ++i) { string256 buffer; BoneProtection BP; .... BP.BonePassBullet = (BOOL) ( atoi( _GetItem(i->second.c_str(), 2, buffer) )>0.5f); .... } } ``` **PVS\-Studio warning:** [V674](https://pvs-studio.com/en/docs/warnings/v674/) The '0\.5f' literal of the 'float' type is compared to a value of the 'int' type\. boneprotections\.cpp 54 The analyzer detected an integer comparison with a real constant\. Perhaps, by analogy, the _atof_ function, not _atoi_ was supposed to be here, but in any case, this comparison should be rewritten so that it doesn't look suspicious\. However, only the author of this code can say for sure if this code is erroneous or not\. ```cpp class IGameObject : public virtual IFactoryObject, public virtual ISpatial, public virtual ISheduled, public virtual IRenderable, public virtual ICollidable { public: .... virtual u16 ID() const = 0; .... } BOOL CBulletManager::test_callback( const collide::ray_defs& rd, IGameObject* object, LPVOID params) { bullet_test_callback_data* pData = (bullet_test_callback_data*)params; SBullet* bullet = pData->pBullet; if( (object->ID() == bullet->parent_id) && (bullet->fly_distflags.ricochet_was)) return FALSE; BOOL bRes = TRUE; if (object){ .... } return bRes; } ``` **PVS\-Studio warning:** [V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'object' pointer was utilized before it was verified against nullptr\. Check lines: 42, 47\. level\_bullet\_manager\_firetrace\.cpp 42 The verification of the _object _pointer against _nullptr _occurs after the _object\-\>ID\(\) _is dereferenced\._ _In the case where _object_ is nullptr, the program will crash\. ```cpp #ifdef _EDITOR BOOL WINAPI DllEntryPoint(....) #else BOOL WINAPI DllMain(....) #endif { switch (ul_reason_for_call) { .... case DLL_THREAD_ATTACH: if (!strstr(GetCommandLine(), "-editor")) CoInitializeEx(NULL, COINIT_MULTITHREADED); timeBeginPeriod(1); break; .... } return TRUE; } ``` **PVS\-Studio warning:** [V718](https://pvs-studio.com/en/docs/warnings/v718/) The 'CoInitializeEx' function should not be called from 'DllMain' function\. xrcore\.cpp 205 In the _DllMain_,_ _we cannot use a part of WinAPI function, including _CoInitializeEx\._ You may read [documentation on MSDN](https://msdn.microsoft.com/en-us/library/windows/desktop/ms695279%28v=vs.85%29.aspx) to be clear on this\. There is probably no definite answer of how to rewrite this function, but we should understand that this situation is really dangerous, because it can cause thread deadlock, or a program crash\. ## Precedence errors ```cpp int sgetI1( unsigned char **bp ) { int i; if ( flen == FLEN_ERROR ) return 0; i = **bp; if ( i > 127 ) i -= 256; flen += 1; *bp++; return i; } ``` **PVS\-Studio warning:** [V532](https://pvs-studio.com/en/docs/warnings/v532/) Consider inspecting the statement of '\*pointer\+\+' pattern\. Probably meant: '\(\*pointer\)\+\+'\. lwio\.c 316 The error is related to increment usage\. To make this expression more clear, let's rewrite it, including the brackets: ```cpp *(bp++); ``` So we'll have a shift not of the content by _bp_ address, but the pointer itself, which is meaningless in this context\. Further on in the code there are fragments of _\*bp_ _\+\=_ _N_ type, made me think that this is an error\. Placing parentheses could help to avoid this error and make the evaluation more clear\. Also a good practice is to use _const_ for arguments that should not changed\. Similar warnings: * V532 Consider inspecting the statement of '\*pointer\+\+' pattern\. Probably meant: '\(\*pointer\)\+\+'\. lwio\.c 354 * V532 Consider inspecting the statement of '\*pointer\+\+' pattern\. Probably meant: '\(\*pointer\)\+\+'\. lwob\.c 80 ```cpp void CHitMemoryManager::load (IReader &packet) { .... if (!spawn_callback || !spawn_callback->m_object_callback) if(!g_dedicated_server) Level().client_spawn_manager().add( delayed_object.m_object_id,m_object->ID(),callback); #ifdef DEBUG else { if (spawn_callback && spawn_callback->m_object_callback) { VERIFY(spawn_callback->m_object_callback == callback); } } #endif // DEBUG } ``` **PVS\-Studio warning:** [V563](https://pvs-studio.com/en/docs/warnings/v563/) It is possible that this 'else' branch must apply to the previous 'if' statement\. hit\_memory\_manager\.cpp 368 In this fragment the else branch is related to the second _if_ because of its right\-associativity, which doesn't coincide with the code formatting\. Fortunately, this doesn't affect the work of the program in any way, but nevertheless, it can make the debugging and testing process much more complicated\. So the recommendation is simple \- put curly brackets in more, or less, complex branches\. ```cpp void HUD_SOUND_ITEM::PlaySound(HUD_SOUND_ITEM& hud_snd, const Fvector& position, const IGameObject* parent, bool b_hud_mode, bool looped, u8 index) { .... hud_snd.m_activeSnd->snd.set_volume( hud_snd.m_activeSnd->volume * b_hud_mode?psHUDSoundVolume:1.0f); } ``` **PVS\-Studio warning:** [V502](https://pvs-studio.com/en/docs/warnings/v502/) Perhaps the '?:' operator works in a different way than it was expected\. The '?:' operator has a lower priority than the '\*' operator\. hudsound\.cpp 108 A ternary conditional operator has a lower precedence than the multiplication operator, that's why the order of operations will be as follows: ```cpp (hud_snd.m_activeSnd->volume * b_hud_mode)?psHUDSoundVolume:1.0f ``` Apparently, the correct code should be the following: ```cpp hud_snd.m_activeSnd->volume * (b_hud_mode?psHUDSoundVolume:1.0f) ``` Expressions containing a ternary operator, several _if\-else_ branches, or operations AND/OR, are all cases where it's better to put extra brackets\. Similar warnings: * V502 Perhaps the '?:' operator works in a different way than was expected\. The '?:' operator has a lower priority than the '\+' operator\. uihudstateswnd\.cpp 487 * V502 Perhaps the '?:' operator works in a different way than was expected\. The '?:' operator has a lower priority than the '\+' operator\. uicellcustomitems\.cpp 106 ## Unnecessary comparisons ```cpp void CDestroyablePhysicsObject::OnChangeVisual() { if (m_pPhysicsShell){ if(m_pPhysicsShell)m_pPhysicsShell->Deactivate(); .... } .... } ``` **PVS\-Studio warning:** [V571](https://pvs-studio.com/en/docs/warnings/v571/) Recurring check\. The 'if \(m\_pPhysicsShell\)' condition was already verified in line 32\. destroyablephysicsobject\.cpp 33 In this case _m\_pPhysicsShell_ gets checked twice\. Most likely, the second check is redundant\. ```cpp void CSE_ALifeItemPDA::STATE_Read(NET_Packet &tNetPacket, u16 size) { .... if (m_wVersion > 89) if ( (m_wVersion > 89)&&(m_wVersion < 98) ) { .... }else{ .... } } ``` **PVS\-Studio warning:** [V571](https://pvs-studio.com/en/docs/warnings/v571/) Recurring check\. The 'm\_wVersion \> 89' condition was already verified in line 987\. xrserver\_objects\_alife\_items\.cpp 989 This code is very strange\. In this fragment we see that a programmer either forgot an expression after _if_ _\(m\_wVersion \> 89\)_, or a whole series of _else\-if_\. This method requires a more scrutinizing review\. ```cpp void ELogCallback(void *context, LPCSTR txt) { .... bool bDlg = ('#'==txt[0])||((0!=txt[1])&&('#'==txt[1])); if (bDlg){ int mt = ('!'==txt[0])||((0!=txt[1])&&('!'==txt[1]))?1:0; .... } } ``` **PVS\-Studio warnings:** * [V590](https://pvs-studio.com/en/docs/warnings/v590/) Consider inspecting the '\(0 \!\= txt\[1\]\) && \('\#' \=\= txt\[1\]\)' expression\. The expression is excessive or contains a misprint\. elog\.cpp 29 * [V590](https://pvs-studio.com/en/docs/warnings/v590/) Consider inspecting the '\(0 \!\= txt\[1\]\) && \('\!' \=\= txt\[1\]\)' expression\. The expression is excessive or contains a misprint\. elog\.cpp 31 The check _\(0_ _\!\=_ _txt\[1\]\)_ is excessive in the expressions of initialization of the _bDlg_ and _mt_ variables\. If we omit it, the expression will be much easier to read\. ```cpp bool bDlg = ('#'==txt[0])||('#'==txt[1]); int mt = ('!'==txt[0])||('!'==txt[1])?1:0; ``` ## Errors in data types ![0405_XRay/image4.png](https://import.viva64.com/docx/blog/0405_XRay/image4.png) ```cpp float CRenderTarget::im_noise_time; CRenderTarget::CRenderTarget() { .... param_blur = 0.f; param_gray = 0.f; param_noise = 0.f; param_duality_h = 0.f; param_duality_v = 0.f; param_noise_fps = 25.f; param_noise_scale = 1.f; im_noise_time = 1/100; im_noise_shift_w = 0; im_noise_shift_h = 0; .... } ``` **PVS\-Studio warning:** [V636](https://pvs-studio.com/en/docs/warnings/v636/) The '1 / 100' expression was implicitly cast from 'int' type to 'float' type\. Consider utilizing an explicit type cast to avoid the loss of a fractional part\. An example: double A \= \(double\)\(X\) / Y;\. gl\_rendertarget\.cpp 245 The value of the expression 1/100 is 0, since it is an operation of integer division\. To get the value 0\.01f, we need to use a real literal and rewrite the expression: 1/100\.0f\. Although, there is still a chance that such behavior was meant to be here, and there is no error\. ```cpp CSpaceRestriction::merge(....) const { .... LPSTR S = xr_alloc(acc_length); for ( ; I != E; ++I) temp = strconcat(sizeof(S),S,*temp,",",*(*I)->name()); .... } ``` **PVS\-Studio warning:** [V579](https://pvs-studio.com/en/docs/warnings/v579/) The strconcat function receives the pointer and its size as arguments\. It is possibly a mistake\. Inspect the first argument\. space\_restriction\.cpp 201 The function _strconcat _gets the buffer size as the first parameter\. _S _buffer is declared as a _LPSTR_, i\.e\. as a pointer to a string\. _sizeof\(S\)_ will be equal to the pointer size in bites, namely _sizeof\(char \*\), _not the number of symbols in the string\._ _To evaluate the length we should use _strlen\(S\)_\. ```cpp class XRCDB_API MODEL { .... u32 status; // 0=ready, 1=init, 2=building .... } void MODEL::build (Fvector* V, int Vcnt, TRI* T, int Tcnt, build_callback* bc, void* bcp) { .... BTHREAD_params P = { this, V, Vcnt, T, Tcnt, bc, bcp }; thread_spawn(build_thread,"CDB-construction",0,&P); while (S_INIT == status) Sleep(5); .... } ``` **PVS\-Studio warning:** [V712](https://pvs-studio.com/en/docs/warnings/v712/) Be advised that compiler may delete this cycle, or make it infinite\. Use volatile variable\(s\) or synchronization primitives to avoid this\. xrcdb\.cpp 100 The compiler can remove the check _S\_INIT \=\= status_ as a measure of optimization, because the _status _variable isn't modified in the loop\. To avoid such behavior, we should use _volatile_ variables, or types of data synchronization between the threads\. Similar warnings: * V712 Be advised that compiler may delete this cycle or make it infinite\. Use volatile variable\(s\) or synchronization primitives to avoid this\. levelcompilerloggerwindow\.cpp 23 * V712 Be advised that compiler may delete this cycle or make it infinite\. Use volatile variable\(s\) or synchronization primitives to avoid this\. levelcompilerloggerwindow\.cpp 232 ```cpp void CAI_Rat::UpdateCL() { .... if (!Useful()) { inherited::UpdateCL (); Exec_Look (Device.fTimeDelta); CMonsterSquad *squad = monster_squad().get_squad(this); if (squad && ((squad->GetLeader() != this && !squad->GetLeader()->g_Alive()) || squad->get_index(this) == u32(-1))) squad->SetLeader(this); .... } .... } ``` **PVS\-Studio warning:** [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'squad\-\>get\_index\(this\) \=\= u32\(\- 1\)' is always false\. The value range of unsigned char type: \[0, 255\]\. ai\_rat\.cpp 480 To understand why this expression is always false, let's evaluate values of individual operands\. u32\(\-1\) is 0xFFFFFFFF or 4294967295\. The type, returned by the method _squad\-\>get\_index\(\.\.\.\.\), _is_ u8,_ thus its maximum value is 0xFF or 255, which is strictly less than u32\(\-1\)\. Consequently, the result of such comparison will always be false\. This code can be easily fixed, if we change the data type to u8: ```cpp squad->get_index(this) == u8(-1) ``` The same diagnostic is triggered for redundant comparisons of unsigned variables\. ```cpp namespace ALife { typedef u64 _TIME_ID; } ALife::_TIME_ID CScriptActionCondition::m_tLifeTime; IC bool CScriptEntityAction::CheckIfTimeOver() { return((m_tActionCondition.m_tLifeTime >= 0) && ((m_tActionCondition.m_tStartTime + m_tActionCondition.m_tLifeTime) < Device.dwTimeGlobal)); } ``` **PVS\-Studio warning:** [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'm\_tActionCondition\.m\_tLifeTime \>\= 0' is always true\. Unsigned type value is always \>\= 0\. script\_entity\_action\_inline\.h 115 The variable _m\_tLifeTime_ is unsigned, thus it is always greater than or equal to zero_\._ It's up to the developer to say if it is an excessive check, or an error in the logic of the program\. The same warning: V547 Expression 'm\_tActionCondition\.m\_tLifeTime < 0' is always false\. Unsigned type value is never < 0\. script\_entity\_action\_inline\.h 143 ```cpp ObjectFactory::ServerObjectBaseClass * CObjectItemScript::server_object (LPCSTR section) const { ObjectFactory::ServerObjectBaseClass *object = nullptr; try { object = m_server_creator(section); } catch(std::exception e) { Msg("Exception [%s] raised while creating server object from " "section [%s]", e.what(),section); return (0); } .... } ``` **PVS\-Studio warning:** [V746](https://pvs-studio.com/en/docs/warnings/v746/) Type slicing\. An exception should be caught by reference rather than by value\. object\_item\_script\.cpp 39 The function_ std::exception::what\(\)_ is virtual, and can be overridden in inherited classes\. In this example, the exception is caught by value, hence the class instance will be copied, and all information about the polymorphic type will be lost\. Accessing _what\(\)_ is meaningless in this case\. The exception should be caught by reference: ```cpp catch(const std::exception& e) { ``` ## Miscelleneous ```cpp void compute_cover_value (....) { .... float value [8]; .... if (value[0] < .999f) { value[0] = value[0]; } .... } ``` **PVS\-Studio warning:** [V570](https://pvs-studio.com/en/docs/warnings/v570/) The 'value\[0\]' variable is assigned to itself\. compiler\_cover\.cpp 260 The variable _value\[0\]_ is assigned to itself_\._ It's unclear, why this should be\. Perhaps a different value should be assigned to it\. ```cpp void CActor::g_SetSprintAnimation(u32 mstate_rl, MotionID &head, MotionID &torso, MotionID &legs) { SActorSprintState& sprint = m_anims->m_sprint; bool jump = (mstate_rl&mcFall) || (mstate_rl&mcLanding) || (mstate_rl&mcLanding) || (mstate_rl&mcLanding2) || (mstate_rl&mcJump); .... } ``` **PVS\-Studio warning:** [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions '\(mstate\_rl & mcLanding\)' to the left and to the right of the '\|\|' operator\. actoranimation\.cpp 290 Most probably, we have an extra check _mstate\_rl_ _&_ _mcLanding,_ but quite often such warnings indicate an error in the logic and enum values that weren't considered\. Similar warnings: * V501 There are identical sub\-expressions 'HudItemData\(\)' to the left and to the right of the '&&' operator\. huditem\.cpp 338 * V501 There are identical sub\-expressions 'list\_idx \=\= e\_outfit' to the left and to the right of the '\|\|' operator\. uimptradewnd\_misc\.cpp 392 * V501 There are identical sub\-expressions '\(D3DFMT\_UNKNOWN \=\= fTarget\)' to the left and to the right of the '\|\|' operator\. hw\.cpp 312 ```cpp RELATION_REGISTRY::RELATION_MAP_SPOTS::RELATION_MAP_SPOTS() { .... spot_names[ALife::eRelationTypeWorstEnemy] = "enemy_location"; spot_names[ALife::eRelationTypeWorstEnemy] = "enemy_location"; .... } ``` **PVS\-Studio warning:** [V519](https://pvs-studio.com/en/docs/warnings/v519/) The variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 57, 58\. relation\_registry\.cpp 58 The analyzer detected that the same variable is assigned with two values in a row\. In this case, it seems that it's just dead code, and it should be removed\. ```cpp void safe_verify(....) { .... printf("FATAL ERROR (%s): failed to verify data\n"); .... } ``` **PVS\-Studio warning:** [V576](https://pvs-studio.com/en/docs/warnings/v576/) Incorrect format\. A different number of actual arguments is expected while calling 'printf' function\. Expected: 2\. Present: 1\. entry\_point\.cpp 41 An insufficient number or arguments is passed to the _printpf_ function: the format '%s' shows that the pointer to a string should be passed\. Such a situation can lead to a memory access error, and to program termination\. ## Conclusion ![0405_XRay/image5.png](https://import.viva64.com/docx/blog/0405_XRay/image5.png) The analysis of X\-Ray Engine detected a good number of both redundant and suspicious code, as well as erroneous and hazardous moments\. It is worth noting that a static analyzer is of great help in detecting errors during the early stages of development, which significantly simplifies the life of a programmer and provides more time for creation of new versions of your applications\.