The ideal way to implement a static code analyzer

One of the main difficulties when using static analysis tools is working with false positives. There are many ways to eliminate false positives using the analyzer settings or changing the code. I took the small Apple II emulator for Windows project and show how to work in practice with the PVS-Studio static analyzer report. I will show you with examples how to correct errors and suppress false positives.

Introduction

I will describe the ideal process of introducing a static analysis methodology into a project. It consists in eliminating all false alarms of the analyzer and real errors so that the analyzer generates 0 warnings. This is the approach we used when working with the Unreal Engine 4 project.

In practice, the ideal approach is rarely possible. Therefore, in a large project, it is wise to use an alternative approach. You can hide all currently available warnings and see only messages related to the new or changed code. For this, the PVS-Studio analyzer has a special mechanism that stores information in a special database. Details can be found in the article: How to implement static analysis in a project with more than 10 megabytes of source code?

So, hiding all the messages, you can carefully monitor the quality of the new code. Finding errors in the new code, you will quickly appreciate the power and benefits of the static analysis methodology. And when you have free time, you can return to hidden warnings and gradually make the necessary changes to the code.

But back to the perfect happy world. Imagine that we have time, and we can safely work out the warnings that the PVS-Studio analyzer generates.

In this article I want to show how to work with analyzer warnings. And we will go the full way - from the first check, to the window in which there will be 0 warnings.

That is why I chose a small project. I could take more, but then I will get tired of writing an article, and you will read it. However, you still get tired. An article even for a small project will turn out to be large, but please pay attention to it. It can help you make better use of our code analyzer.

The experimental mouse was the Apple II emulator for Windows project . The choice fell on him completely by accident. Therefore, we will not dwell on it. I didn’t care which project to take, the main thing was to be small, but at the same time there was something interesting in it.

Project Features:

• Source code size: 3 megabytes.
• Number of lines of code: 85700.
• Analysis time (8 processor cores): 30 seconds.

First start

After the analyzer’s first start, we have the following warnings:


Figure 1. Warnings issued when the PVS-Studio analyzer was first started for the Apple II emulator for Windows project.

In this article, I will only discuss level 1 and level 2 warnings related to general analysis (GA). It would be possible to “win” and Level 3, but then the article will drag on. We look at level 3 very quickly, and I will give some explanations, but I will not correct anything.

Microoptimization (OP): now not interesting.

About 64-bit diagnostics: the project does not have a 64-bit configuration. Not interested.

After checking the project, I sorted all the warnings by code. This can be done by clicking on the “Code” column (see Figure N2).


Figure 2. PVS-Studio message window. Messages are sorted by diagnostic number.

Code sorting makes messaging easier. Groups of messages of the same type arise. Understanding the cause of one message, you can easily and quickly process the rest.

Note. The reader may wonder why not do this sorting right away. The fact is that I want to allow users to look at messages in the analysis process. If you sort them, then new messages in the analysis process will not be added to the end, but to different places in the list. As a result, the messages will “jump”. It will be impossible to work with such a “twitching” list.

Work with analyzer messages

There are three projects in the solution (they are visible in the Solution Explorer window in Figure N2). Two of them, zlib and zip_lib, are not interesting to check. Therefore, they must be excluded from the analysis. Actually, it is enough to exclude zip_lib, since zlib is already added to the exceptions by default. This is done in the PVS-Studio settings window ( Don't Check Files section ):


Figure N3. Excluded zip_lib from check.

I ruled out an unnecessary project in advance. However, this is easy to do after verification. And it’s not necessary to go to settings for this. The context menu has an item that allows you to conveniently hide all messages related to a file or a specific directory. It is very comfortable. I recommend to get acquainted with the article " PVS-Studio for Visual C ++". It describes this and other features that will help to use the tool effectively.

So, everything is ready to start working on messages. Let's start with the diagnostics under the number V501 and go further. In total, we will consider 32 + 49 = 81 messages. This is quite a lot. Therefore, in some places I will write in detail, and in some places I will be brief.

Macro false alarm xxxxxREG

The first 6 messages are due to the complex macros ADDXXREG, ADCHLREG, SBCHLREG, SBCHLREG. When they are expanded, redundant constructions arise, and the analyzer displays, for example, the following message:

V501 There are identical sub-expressions to the left and to the right of the '^' operator: (tmp >> 8) ^ reg_ixh ^ reg_ixh z80.cpp 3444

The ADDXXREG macro is very large and consists of other macros. Therefore, I will not cite it in an article.

It is important for us that the XOR operation is performed twice with the variable reg_ixh. Accordingly, the expression can be simplified to (tmp >> 8). However, there is no mistake here. It just turned out to be an excess expression when substituting certain macro arguments:

ADDXXREG (reg_ixh, reg_ixl, reg_ixh, reg_ixl, 15, 2);

These are false positives, and we must eliminate them. Suppress all warnings associated with them. To do this, in the header file where these macros are declared, I added the following comments:

• // - V: ADDXXREG: 501
• // - V: ADCHLREG: 501
• // - V: SBCHLREG: 501
• // - V: SBCHLREG: 501

Details about this warning suppression mechanism can be found in the corresponding section of the documentation .

In principle, you can get by with one comment. The names of all macros have “REG”. Therefore, you can write one comment: // - V: REG: 501. It will suppress V501 warnings in the lines where “REG” will occur. But this is bad, since you can accidentally hide a useful message that is not related to the macros listed above. You can improve the situation a little by adding a bracket to search: // - V: REG (: 501. In this case, I think that you should not be lazy and write 4 comments.

Error in sprintf () function parameters

sprintf( sText, "%s %s = %s\n"
  , g_aTokens[ TOKEN_COMMENT_EOL  ].sToken
  , g_aParameters[ PARAM_CATEGORY ].m_sName
  , g_aParameters[ eCategory ]
  );

Warning: V510 The 'sprintf' function is not expected to receive class-type variable as fifth actual argument. debug.cpp 2300

Indeed, the fifth actual argument is a structure of type Command_t. Apparently, g_aParameters [eCategory] .m_sName should be used as an argument. Made the appropriate changes to the code.

Bad Smelling ZeroMemory ()

The following message tells us that one array is not full: V512 A call of the 'memset' function will lead to underflow of the buffer 'pHDD-> hd_buf'. harddisk.cpp 491

BYTE  hd_buf[HD_BLOCK_SIZE+1]; // Why +1?
ZeroMemory(pHDD->hd_buf, HD_BLOCK_SIZE);

The last byte is not reset. I can’t say whether this is a mistake or not. Pay attention to the comment. Perhaps even the developers themselves do not know what size the array should be and whether to completely zero it out.

This code is said to be odorless. It is not necessarily mistaken, but suspicious and can cause a mistake in the future.

I will simply suppress this warning with a comment. You can make changes to the file yourself, or you can use the menu item “Mark selected messages as False Alarms”:


Figure 3. Adding a comment to the code to suppress the warning.

Selecting this item, the analyzer will insert a comment into the code:

ZeroMemory(pHDD->hd_buf, HD_BLOCK_SIZE); //-V512

False positive on memcpy () function call

unsigned char random[ 256 + 4 ];
memcpy( &memmain[ iByte ], random, 256 );

The memcpy () function copies only part of the 'random' buffer. The analyzer considers this suspicious and honestly reports it. In this case, he is wrong, and there is no mistake. I suppressed the warning, as in the previous case, using a comment. This is not very beautiful, but I don’t know what to do in someone else’s code.

Extra actions

nAddress_ = 0;
nAddress_ = (unsigned)*(LPBYTE)(mem + nStack);
nStack++;
nAddress_ += ((unsigned)*(LPBYTE)(mem + nStack)) << 8;

Warning: V519 The 'nAddress_' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 568, 569. debugger_assembler.cpp 569

The analyzer noticed that the variable nAddress_ was assigned various values ​​several times in a row. There is no mistake here, just extra code. I deleted the first line, where the variable is assigned the value 0. Another option to get rid of the warning is to replace the second assignment with "+ =".

A similar situation can be observed in two more files: The

video.cpp file (see lines 3310 and 3315). I removed the extra operation "pSrc + = nLen;".

Debug.cpp file (see line 5867 and 5868). Replaced:

char *p = sLine;
p = strstr( sLine, ":" );

on the

char *p = strstr( sLine, ":" );

I will not dwell on these fragments.

Error in switch statement

The following V519 diagnostics already indicate a real serious error. Although the mistake is classical, and everyone knows about it, we meet it again and again in various programs.

switch( c )
{
  case '\\':
    eThis = PS_ESCAPE;
  case '%':
    eThis = PS_TYPE;
    break;
  default:
    sText[ nLen++ ] = c;
    break;
}

Warning: V519 The 'p' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 5867, 5868. debug.cpp 5868

After "eThis = PS_ESCAPE;" no 'break' statement. Because of this, the value of the 'eThis' variable will immediately change to PS_STYPE. This is a clear mistake. To fix it, I added the 'break' statement.

Error: always false condition

inline static ULONG ConvertZ80TStatesTo6502Cycles(UINT uTStates)
{
  return (uTStates < 0) ?
      0 : (ULONG) ((double)uTStates / uZ80ClockMultiplier);
}

Warning: V547 Expression 'uTStates <0' is always false. Unsigned type value is never <0. z80.cpp 5507

The programmer wanted to protect himself from the case when a negative value was passed to the function. However, since the 'uTStates' variable is unsigned, protection will not work.

I added an explicit cast to type 'INT':

return ((INT)uTStates < 0) ?
    0 : (ULONG) ((double)uTStates / uZ80ClockMultiplier);

Excessive alertness of the analyzer

In the next function, the analyzer worries that an array may go out of bounds.

void SetCurrentImageDir(const char* pszImageDir)
{
  strcpy(g_sCurrentDir, pszImageDir);
  int nLen = strlen( g_sCurrentDir );
  if( g_sCurrentDir[ nLen - 1 ] != '\\' )
  ....
}

Warning: V557 Array underrun is possible. The value of 'nLen - 1' index could reach -1. applewin.cpp 553

If you pass an empty string to a function, then its length will be zero. Then there will be an overflow of the array: g_sCurrentDir [0 - 1].

The analyzer does not know whether such a situation is possible or not. Therefore, just in case, and warns.

I don’t know whether such a situation is possible or not. If possible, the analyzer found an error. If not, this is a false positive.

I decided to consider this a false positive. However, it will be more useful to improve the code rather than adding a comment to suppress the warning. Therefore, let an additional check be added to the function:

if (nLen == 0)
  return;

There is one more place where, theoretically, an array can go out of bounds. But we must try not to turn the article into a thick reference. Therefore, I will not describe this warning and just suppress it with the help of a comment. See the same file (line 556).

Assignment instead of comparison

if ((bytenum == 3) && (byteval[1] = 0xAA))
{

Warning: V560 A part of conditional expression is always true: (byteval [1] = 0xAA). diskimagehelper.cpp 439

I'm sure I wanted to perform the '==' operation, not the '='. If you need assignment, it would be much more natural to write like this:

if (bytenum == 3)
{
  byteval[1] = 0xAA;

Therefore, this is a mistake, and it should be fixed:

if ((bytenum == 3) && (byteval[1] == 0xAA))

Macro false positive

if ((TRACKS_MAX>TRACKS_STANDARD) && ....)

Warning: V560 A part of conditional expression is always true: ((35 + 5)> 35). diskimagehelper.cpp 548

If you expand the macros, you get the expression ((35 + 5)> 35). The expression is always true, but it is not a mistake.

This is the case when I don’t even know what to do. The schema and just suppress the false positive using the comment: // - V560.

Extra variable

During refactoring, sometimes “lost” variables remain. They are somehow used in the code, but actually are no longer needed. Apparently this is exactly what happened with the bForeground variable:

BOOL    bForeground;
....
bForeground = FALSE;
....
if( bForeground )
  dwCoopFlags |= DISCL_FOREGROUND;
else
  dwCoopFlags |= DISCL_BACKGROUND;
....
if( hr == DIERR_UNSUPPORTED && !bForeground && bExclusive )

And more, the 'bForeground' variable is not changed or used anywhere. This results in a warning: V560 A part of conditional expression is always true:! BForeground. mouseinterface.cpp 690

This is an interesting example for philosophy. Is it a false positive or not? Even a person is difficult to give an answer. The analyzer is right in detecting an anomaly. But from a human point of view, this fragment may be an incomplete code. And then everything is in order.

We assume that this is another example of a "code with a smell." I removed the 'bForeground' variable from the code.

Undefined behavior

*(mem+addr++) =
  (opcode >= BENCHOPCODES) ? 0x00 : ((addr >> 4)+1) << 4;

Warning: V567 Undefined behavior. The 'addr' variable is modified while being used twice between sequence points. cpu.cpp 564

It is not known how the expression will be evaluated:

• Perhaps the variable 'addr' will increase first, and then it will be used on the right side of the expression.
• And maybe vice versa.

The following code will be correct:

*(mem+addr) =
  (opcode >= BENCHOPCODES) ? 0x00 : ((addr >> 4)+1) << 4;
addr++;

Invalid arguments when calling the wsprintf () function and similar

There are several errors associated with passing the wrong amount of actual arguments to the formatted output function. In total, 10 such errors were found, but we will consider only one:

wsprintf( sText, TEXT("%s full speed Break on Opcode: None")
  , sAction
  , g_iDebugBreakOnOpcode
  , g_aOpcodes65C02[ g_iDebugBreakOnOpcode ].sMnemonic
);

Warning: V576 Incorrect format. A different number of actual arguments is expected while calling 'wsprintfA' function. Expected: 3. Present: 5. debug.cpp 939

When forming a string, the last two parameters are not taken into account. It is difficult for me, as an outsider, to say these parameters are superfluous, or a mistake is made in the format string.

I decided that the parameters are superfluous, and deleted them.

Similar problems are observed in the following code sections:

• Expected: 8. Present: 9. debug.cpp 7377
• Expected: 3. Present: 4. debugger_help.cpp 1263
• Expected: 3. Present: 4. debugger_help.cpp 1265
• Expected: 3. Present: 4. debugger_help.cpp 1267
• Expected: 3. Present: 4. debugger_help.cpp 1282
• Expected: 3. Present: 4. debugger_help.cpp 1286
• Expected: 3. Present: 4. debugger_help.cpp 1288
• Expected: 5. Present: 4. debugger_help.cpp 1332
• Expected: 3. Present: 4. frame.cpp 691
• Expected: 3. Present: 4. frame.cpp 695

There are also a couple of places where "% 08X" is used to print the pointer values. On a 32-bit system, this works in practice. But on a 64-bit system only part of the pointer will be printed. The correct solution is to use "% p". Relevant code sections:

• To print the value of pointer the '% p' ​​should be used. tfe.cpp 507
• To print the value of pointer the '% p' ​​should be used. tfe.cpp 507

False positives on double comparisons

Although the analyzer was not at fault, it produced two false positives on repeated conditions. Consider one case:

if (nAddress <= _6502_STACK_END)
{
  sprintf( sText,"%04X: ", nAddress );
  PrintTextCursorX( sText, rect );
}
if (nAddress <= _6502_STACK_END)
{
  DebuggerSetColorFG( DebuggerGetColor( FG_INFO_OPCODE ));
  sprintf(sText, "  %02X",(unsigned)*(LPBYTE)(mem+nAddress));
  PrintTextCursorX( sText, rect );
}

Warning: V581 The conditional expressions of the 'if' operators located alongside each other are identical. Check lines: 2929, 2935. debugger_display.cpp 2935

No error. The programmer simply divided the two sets of actions. From the point of view of the analyzer, such a code is suspicious. Suddenly the conditions should be different? Nevertheless, something needs to be done with a false positive. I decided to combine the two conditional statements into one:

if (nAddress <= _6502_STACK_END)
{
  sprintf( sText,"%04X: ", nAddress );
  PrintTextCursorX( sText, rect );
  DebuggerSetColorFG( DebuggerGetColor( FG_INFO_OPCODE ));
  sprintf(sText, "  %02X",(unsigned)*(LPBYTE)(mem+nAddress));
  PrintTextCursorX( sText, rect );
}

I think the readability of the code did not suffer from this, but along the way we got rid of a false positive.

The second case is similar: V581 The conditional expressions of the 'if' operators located alongside each other are identical. Check lines: 2237, 2245. debugger_display.cpp 2245


Figure 5. In the middle of a long article, it is recommended to insert a picture so that the reader rests. I don’t know which article image to insert. Therefore, here is a cat for you.

Pointer dereferencing before validation

In total, the analyzer issued 3 warnings on this topic. Unfortunately, the text of the program in these places is rather confusing. Therefore, for simplicity, I will not provide real code, but pseudocode. For the first 2 warnings, the code looks something like this:

int ZEXPORT unzGetGlobalComment(char *szComment)
{
  ....
  if (A)
  {
    *szComment='\0';
     return UNZ_ERRNO;
  }
  ....
  if ((szComment != NULL) && X)
  ....
}

Warning: V595 The 'szComment' pointer was utilized before it was verified against nullptr. Check lines: 1553, 1558. unzip.c 1553

As you can see, the passed pointer 'szComment' can be NULL. This is evidenced by a check (szComment! = NULL).

However, there is a section of code in which the pointer is boldly dereferenced without checking. This is dangerous. It is possible that in practice there are never situations when 'szComment' is 0. But the code is dangerous and should be fixed.

Similar: V595 The 'pToken_' pointer was utilized before it was verified against nullptr. Check lines: 811, 823. debugger_parser.cpp 811

But with the last, third case, everything is more complicated. I'm already tired of proving that such a code is incorrect and should be fixed. The function is short, so I will give it in its entirety

bool ArgsGetValue ( Arg_t *pArg,
                    WORD * pAddressValue_, const int nBase )
{
  TCHAR *pSrc = & (pArg->sArg[ 0 ]);
  TCHAR *pEnd = NULL;
  if (pArg && pAddressValue_)
  {
    *pAddressValue_ =
      (WORD)(_tcstoul( pSrc, &pEnd, nBase) & _6502_MEM_END);
    return true;
  }
  return false;
}

Warning: V595 The 'pArg' pointer was utilized before it was verified against nullptr. Check lines: 204, 207. debugger_parser.cpp 204 The

pointer 'pArg' may be zero, as evidenced by the condition “if (pArg && pAddressValue_)”. But before the pointer is checked, it is used in the expression:

TCHAR *pSrc = & (pArg->sArg[ 0 ]);

This expression leads to undefined program behavior. You cannot dereference null pointers.

Many people argue that in this code there is no memory access, but just some address is calculated. Therefore, there is no problem. However, this is too narrow an interpretation of indefinite behavior. There is no need to guess at all how the compiler will behave and how the code will work. So you can’t write, and it makes no sense to discuss why.

Undefined behavior in such code is not only a reference to the zero memory address (which really may not exist). For example, the compiler is quite entitled to reduce the check condition to “if (pAddressValue_)”. Since there is an expression “pArg-> xxx”, then the pointer is definitely not null and does not need to be checked.

There is no point in discussing this issue in more detail. I suggest getting acquainted with a special article: Dereferencing a null pointer leads to undefined behavior .

The code is easy to fix. It is enough to transfer the declaration of the variable inside the 'if'.

Frightening expression

The analyzer was confused by the following expression:

if ((cx > 4) & (cx <= 13))

Warning: V602 Consider inspecting the '(cx> 4)' expression. '>' possibly should be replaced with '>>'. debug.cpp 8933

The analyzer sees that the operator '&' is used, the operands of which are the type 'bool' used. This is strange. Usually in such cases it is customary to use the logical operator '&&'.

The '&' operator is commonly used for bitwise operations. Therefore, the analyzer suggested the possibility that they also wanted to work with bits here:

if ((cx >> 4) & (cx <= 13))

The analyzer is too smart and wrong. However, the programmer’s fault is also here. This code is with a smell. It will be much more natural to write:

if (cx > 4 && cx <= 13)

Unspecified behavior and horror in macros

It is not clear what the result will be when shifting negative values ​​to the right. It’s better not to do this, as code behavior may change on another compiler.

const short SPKR_DATA_INIT = (short)0x8000;
if (g_nSpeakerData == (SPKR_DATA_INIT >> 2))

Warning: V610 Unspecified behavior. Check the shift operator '>>'. The left operand 'SPKR_DATA_INIT' is negative. speaker.cpp 450

Output: You can declare the constant SPKR_DATA_INIT to be unbanked. True, then you will need to make a few more minor changes so that the compiler and the analyzer do not receive warnings about comparing signed / unsigned numbers.

The analyzer found 3 more such dangerous places:

• The left operand 'SPKR_DATA_INIT' is negative. speaker.cpp 453
• The left operand '~ 0x180' is negative. tfe.cpp 869
• The left operand '~ 0x100' is negative. tfe.cpp 987

By the way, when I started to edit the last two warnings, I found 2 more errors. We can say that the analyzer helped to find them in an indirect way.

Here's how the macro is used:

SET_PP_16(TFE_PP_ADDR_SE_BUSST, busst & ~0x180);

It expands to a long string. Therefore, I will show only part of it:

..... = (busst & ~0x180 >> 8) & 0xFF; .....

The priority of the shift operator >> is greater than the priority of the & operator. See table: operation priorities .

The programmer expected the code to work like this:

..... = ((busst & ~0x180) >> 8) & 0xFF; .....

But in fact, it works like this:

..... = (busst & (~0x180 >> 8)) & 0xFF; .....

That's why the PVS-Studio analyzer warns: "the left operand '~ 0x180' is negative."

This is how dangerous it is to use macros!

Security Holes

The project uses the sprintf (), wsprintf () functions, and so on, in a very dangerous way. In short, the functions are used as follows:

sprintf(buf, STR);

If the STR string contains control characters such as "% s", the consequences will be unpredictable.

Typically, this code is considered a security hole (see details ).

However, for the emulator, I think this is not critical. No one will attack him. However, such a code is dangerous in itself. It can easily lead to a crash of the program or to its malfunction.

The right thing to do is: sprintf (buf, "% s", STR);

The analyzer found quite a few dangerous function calls. In total, he issued 21 warnings .

Opposite conditions

// TO DO: Need way of determining if DirectX init failed
if (soundtype != SOUND_WAVE)
{
  if (soundtype == SOUND_WAVE)
    soundtype = SOUND_SMART;

Warning: V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 270, 272. speaker.cpp 270

Judging by the comment, the code has not been added. It’s hard to say what to do with this code. I decided to comment out the second pointless 'if':

if (soundtype != SOUND_WAVE)
{
  //if (soundtype == SOUND_WAVE)
  //  soundtype = SOUND_SMART;

Bad alignment code

The code looks as if both actions are related to the if statement:

{
  if ((Slot4 == CT_MockingboardC) || (Slot4 == CT_Phasor))
    m_PropertySheetHelper.GetConfigNew().m_Slot[4] = CT_Empty;
    m_PropertySheetHelper.GetConfigNew().m_Slot[5] = CT_SAM;
}

Warning: V640 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. pagesound.cpp 229

As I understand it, there is no error in the code. However, this is not a false positive. The analyzer is absolutely right when warning about such a code. Be sure to correct the alignment:

{
  if ((Slot4 == CT_MockingboardC) || (Slot4 == CT_Phasor))
    m_PropertySheetHelper.GetConfigNew().m_Slot[4] = CT_Empty;
  m_PropertySheetHelper.GetConfigNew().m_Slot[5] = CT_SAM;
}

Incorrect work with strncat () function

strncat( sText, CHC_DEFAULT, CONSOLE_WIDTH );
strncat( sText, pHelp      , CONSOLE_WIDTH );

Warning: V645 The 'strncat' function call could lead to the 'sText' buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold. debugger_help.cpp 753

The third argument to the function is the number of characters that can still be added to the string. Therefore, it is correct and safe to do so:

strncat( sText, CHC_DEFAULT, sizeof(sText) - strlen(sText) - 1);
strncat( sText, pHelp      , sizeof(sText) - strlen(sText) - 1);

Details can be found by reading the description of the V645 diagnostic .

Extra checks

For a very long time, the 'new' operator has thrown an exception std :: bad_alloc if it cannot allocate memory. However, in programs you can still find unnecessary checks:

BYTE* pNewImageBuffer = new BYTE [uNewImageSize];
_ASSERT(pNewImageBuffer);
if (!pNewImageBuffer)
  return false;

Warning: V668 There is no sense in testing the 'pNewImageBuffer' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. diskimagehelper.cpp 197

_ASSERT and the validation can and should be removed. Here they make no sense.

A similar situation:

• mouseinterface.cpp 175
• serialcomms.cpp 839
• savestate.cpp 108
• savestate.cpp 218
• speech.cpp 40

Self-Declaring System Types

The project consistently defines some data types:

typedef unsigned long ULONG;
typedef void *LPVOID;
typedef unsigned int UINT;

There is no obvious error here. We assume that this is a “code with a smell” and suppress warnings using the comment // - V677.

Violated the "Law of the Big Two"

For example, operator = is declared in the CConfigNeedingRestart class, but there is no copy constructor. This violates the " Big Two Law ."

The class is large enough, so I will not give code fragments here. Take a word.

In this class, all fields are simple types, so your own operator = is not needed at all. The class will be successfully copied automatically.

With the Disk_t class, the situation is similar. Both here and there you can remove operator =.

Analyzer Warnings:

• V690 The 'CConfigNeedingRestart' class implements the '=' operator, but lacks a copy constructor. It is dangerous to use such a class. config.h 7
• V690 The 'Disk_t' class implements the '=' operator, but lacks a copy constructor. It is dangerous to use such a class. disk.cpp 74

Typo

int nHeight=nHeight=g_aFontConfig[ FONT_CONSOLE ]._nFontHeight; 

Warning: V700 Consider inspecting the 'T foo = foo = ...' expression. It is odd that variable is initialized through itself. debugger_display.cpp 1226

Just a typo. Replaced by:

int nHeight = g_aFontConfig[ FONT_CONSOLE ]._nFontHeight;

Excessive analyzer concern about enumerations

The 'AppMode_e' enumeration contains the following named constants: MODE_LOGO, MODE_PAUSED, MODE_RUNNING, MODE_DEBUG, MODE_STEPPING.

The analyzer worries that not all of them are used in this switch ():

switch (g_nAppMode)
{
  case MODE_PAUSED  : _tcscat(.....); break;
  case MODE_STEPPING: _tcscat(.....); break;
}

Warning: V719 The switch statement does not cover all values ​​of the 'AppMode_e' enum: MODE_DEBUG, MODE_LOGO, MODE_RUNNING. frame.cpp 217

In this example, I am even a little ashamed of the analyzer. Empirical algorithms failed. False alarm. There are several ways to fix it. For example, you can add the default branch.

switch (g_nAppMode)
{
  case MODE_PAUSED  : _tcscat(.....); break;
  case MODE_STEPPING: _tcscat(.....); break;
  default: break;
}

Another similar false positive: V719 The switch statement does not cover all values ​​of the 'AppMode_e' enum: MODE_DEBUG, MODE_LOGO. frame.cpp 1210

I promised to take a quick look at the warnings of the third level.

We do not recommend (at least in the early stages) to drop into level 3 at all. There are many false, uninteresting or specific messages. Now this is the situation.

For example, there are quite a few V601 warnings here.

inline int IsDebugBreakpointHit()
{
  if ( !g_bDebugNormalSpeedBreakpoints )
    return false;
  return _IsDebugBreakpointHit();
}

Warning: V601 The 'false' value is implicitly cast to the integer type. debug.h 210

The function returns the type 'int'. But it says "return false".

The analyzer is right to find fault with this code, but in practice, this is very rarely hidden by a real error. Therefore, this warning fell to level 3.

Now an example of specific diagnostics:

double g_fClksPerSpkrSample;
....
if ((double)g_nRemainderBufferSize != g_fClksPerSpkrSample)

Warning: V550 An odd precise comparison. It's probably better to use a comparison with defined precision: fabs (A - B)> Epsilon. speaker.cpp 197

Whether this code is valid or not depends very much on the application and on what values ​​are placed in variables of type 'double'.

A number of users are delighted with this diagnosis. Others say that they put integers in doubles and know what they do when they are compared. You will not please everyone.

Run after error editing

Now, having corrected all the messages (level 1 and 2), we can restart the analyzer. We expect the result - all warnings have disappeared (see Figure 6).


Figure 6. There are no more Level 1 and Level 2 alerts.

This is an ideal option, applicable only to small projects. Nevertheless, I hope that I was able to show that there is nothing complicated in working with analyzer messages. Although some of the messages were false, there was no difficulty with them, and all of them were eliminated.

To summarize

We are often asked how many false positives our analyzer emits. We have no answer. It is very difficult to collect such statistics, and it gives little. The number of false positives varies greatly from project to project.

There is still a problem with data interpretation. For example, due to an unsuccessful macro, which is actively used throughout the project, the number of false positives can be 20 times more than useful messages. But it does not matter. It is enough to suppress warnings in this macro, and the number of false messages can immediately decrease by 90%.

The next difficulty to answer is that programmers do not take into account that some warnings are difficult to classify. These messages do not detect errors, but reveal a "code with a smell." Such a code should be corrected, since although it now works correctly, in the future it may cause a defect. The article showed several examples of such diagnostics.

But programmers gravitate towards binary logic. And they insist on receiving an answer: “Is this a false message? Well no?". If you carefully read the article, I hope you will not raise the question so strictly.

As you can see, it’s difficult to talk generally about the number of false positives. But if you take a specific small project, then you can give an answer just for it.

Summary of diagnostic messages generated by PVS-Studio analyzer for the Apple II emulator for Windows project:

• Total number of messages issued (General Analysis, first and second level): 81
• Found errors: 57
• Found fragments of "code with a smell" that should be fixed: 9
• False positives: 15

To summarize the percentage:

• Messages indicating errors: 70%
• Messages indicating a “wraparound code”: 11%
• False positives: 19%

Conclusion

Try the PVS-Studio static analyzer on your project. You can download the demo version at: http://www.viva64.com/en/pvs-studio-download/

And I suggest recommending our colleagues and friends to try our static analyzer. Please write a message on twitter or any other news feed. Thanks.

PS You can follow our new articles and generally learn news from the C / C ++ world if you subscribe to my twitter: https://twitter.com/Code_Analysis

Thank you all for your attention.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey karpov. An Ideal Way to Integrate a Static Code Analyzer into a Project .