PVS-Studio warning: V533 It is likely that the wrong variable is being incremented inside the 'for' operator. Consider reviewing 'i'. mxqmetric.cpp 76

The analyzer detected that in the nested for loop, the variable i gets incremented, but another variable - j gets checked, which leads to an infinite loop. Most likely, a programmer just forgot to change it.

PVS-Studio 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 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.

PVS-Studio warning: 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.

PVS-Studio warning: 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:

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.

PVS-Studio warning: 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.

PVS-Studio warning: 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.

PVS-Studio warnings:

• V560 A part of conditional expression is always true: 0x000000FF. nvi_image.cpp 170
• V560 A part of conditional expression is always true: 0x000000FF. nvi_image.cpp 171
• V560 A part of conditional expression is always true: 0x000000FF. nvi_image.cpp 172
• 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:

PVS-Studio warning: 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.

PVS-Studio warning: 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 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

PVS-Studio warning: 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:

The following error is connected with incorrectly formed logical expression.

PVS-Studio warning: 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".

PVS-Studio warning: 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.

PVS-Studio warning: 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.

PVS-Studio warning: 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 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

PVS-Studio warning: 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:

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

PVS-Studio warning: 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.

PVS-Studio warning: 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:

Apparently, the correct code should be the following:

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

PVS-Studio warning: 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.

PVS-Studio warning: 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.

PVS-Studio warnings:

• V590 Consider inspecting the '(0 != txt[1]) && ('#' == txt[1])' expression. The expression is excessive or contains a misprint. elog.cpp 29
• 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.

Errors in data types

PVS-Studio warning: 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.

PVS-Studio warning: 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).

PVS-Studio warning: 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

PVS-Studio warning: 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:

The same diagnostic is triggered for redundant comparisons of unsigned variables.

PVS-Studio warning: 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

PVS-Studio warning: 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:

Miscelleneous

PVS-Studio warning: 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.

PVS-Studio warning: 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

PVS-Studio warning: 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.

PVS-Studio warning: 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

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.

By Pavel Belikov