False positive handling in PVS-Studio and CppCat

I recently decided to re-test the Newton Game Dynamics physics engine. The project code is of high quality. Therefore, there were almost no warnings that revealed errors. But there were several dozen false positives. There seems to be nothing to write about. But the idea occurred to me that it was possible to write about how to work with false positives, and how to prevent them from happening. The Newton Game Dynamics project seemed to me a suitable candidate for this.

Project Verification

PVS-Studio analyzer generates the following number of warnings:

48 first level; 79 second level; 261 third level (off by default).

This is a general warning ( GA ) warning .

A total of 127 warnings are recommended for viewing. Exactly as many messages are issued by the CppCat analyzer . The article does not distinguish between PVS-Studio and CppCat analyzers. In general, they provide the same mechanisms for suppressing false positives. In PVS-Studio there are slightly more of them, but this does not change the overall picture.

Note. What is the difference between the functionality of PVS-Studio and CppCat here .

It took me a little over three hours to write out all the necessary examples for the article and get rid of all the warnings. I think if I didn’t write out examples, I would manage in an hour. So the complexity of combating false positives is exaggerated. Yes, they interfere and distract. Yes, if the project is large, then there are a lot of false warnings. However, it is not at all difficult to get rid of them.

Total. CppCat generates 0 warnings. PVS-Studio also does not produce anything. You can, of course, enable level 3 or 64-bit diagnostics, but this is not so interesting. For starters, it’s very good to get rid of the recommended warnings. This is a very big step towards improving the quality of code. This is where you need to start. If you turn on all warnings right away, you don’t have the strength to go straight to the end. This, incidentally, is the main mistake of beginners. “More” does not mean “better.”

View Report

PVS-Studio and CppCat analyzers do not group diagnostic messages into groups and do not sort them. With regular use, this is not necessary. If the analyzer detects 2-3 errors in the new code, then there is nothing to combine and sort here. Only unnecessary complication of the interface.

When you just start working with the tool, you can sort the messages by diagnostic number. This is done by clicking on the heading of the column with the diagnostic number. Automatically, we did not do such sorting. Warnings are displayed in file analysis order. This allows you to start watching messages without waiting for the analysis to stop. If they are sorted, then while checking, the messages in the table will “jump”, and it will be impossible to work with them.

So, at the initial stages, sorting by type of warning (diagnostic number) will be useful. I will do so. This will allow me to quickly identify the same type of false positives and exclude them. This can greatly simplify the work and reduce the initial setup time.

Work with alerts

If any of the methods for suppressing false positives seems insufficiently clear, we suggest that you familiarize yourself with the corresponding section of the documentation:

• For PVS-Studio - Suppression of false warnings .
• For CppCat, the documentation comes with the distribution kit.

Warning N1, N2

void
dgWorldDynamicUpdate::CalculateJointsVelocParallelKernel (....)
{
  ....
  dgVector velocStep2(velocStep.DotProduct4(velocStep));
  dgVector omegaStep2(omegaStep.DotProduct4(omegaStep));
  dgVector test((velocStep2 > speedFreeze2) |
                 (omegaStep2 > omegaStep2));
  ....
}

Warning: V501 There are identical sub-expressions to the left and to the right of the '>' operator: omegaStep2> omegaStep2 dgworlddynamicsparallelsolver.cpp 546

The expression "omegaStep2> omegaStep2" looks suspicious. It’s hard for me to judge whether there is a mistake or not. Since this comparison is also found in another file, then, probably, this is still not a mistake, but it is intended.

Let there be no mistake. I marked these two places with a comment:

dgVector test((velocStep2 > speedFreeze2) |
               (omegaStep2 > omegaStep2)); //-V501

Now, at this point warning V501 will not be issued.

Warning N3

dgInt32
dgWorld::CalculatePolySoupToHullContactsDescrete(....) const
{
  ....
  dgAssert (dgAbsf(polygon.m_normal % polygon.m_normal -
            dgFloat32 (1.0f)) < dgFloat32 (1.0e-4f));
  ....
}

Warning: V501 There are identical sub-expressions to the left and to the right of the '%' operator: polygon.m_normal% polygon.m_normal dgnarrowphasecollision.cpp 1921

The analyzer here is both right and wrong. On the one hand, the expression "polygon.m_normal% polygon.m_normal" is really very suspicious. But the analyzer does not realize that this is a test for checking the operator '%' implemented in the class. In fact, the code is correct. We will help the analyzer with a comment:

dgAssert (dgAbsf(polygon.m_normal % polygon.m_normal - //-V501
          dgFloat32 (1.0f)) < dgFloat32 (1.0e-4f));

Warning N4

staticvoidPopupateTextureCacheNode(dScene* const scene){
  ....
  if (!(info->IsType(dSceneCacheInfo::GetRttiType()) ||
        info->IsType(dSceneCacheInfo::GetRttiType()))) {
  ....
}

Warning: V501 There are identical sub-expressions 'info-> IsType (dSceneCacheInfo :: GetRttiType ())' to the left and to the right of the '||' operator. dscene.cpp 125

The same thing is checked twice. We assume that the second check is unnecessary. So I fixed the code as follows:

if (!(info->IsType(dSceneCacheInfo::GetRttiType()))) {

Warning N5

dFloat dScene::RayCast (....) const
{
  ....
  dFloat den = 1.0f /
    ((globalP1 - globalP0) % (globalP1 - globalP0)); //-V501
  ....
}

Warning: V501 There are identical sub-expressions '(globalP1 - globalP0)' to the left and to the right of the '%' operator. dscene.cpp 1280 The

variables globalP0 and globalP1 are instances of the class 'dVector'. Therefore, the code makes sense. The analyzer worries in vain. Let's mark the code:

dFloat den = 1.0f /
  ((globalP1 - globalP0) % (globalP1 - globalP0)); //-V501

Although the analyzer is wrong, this code cannot be called beautiful. I think you can get special functions for such cases or something else.

Warning N6 - N15

dgInt32 dgCollisionCompound::CalculateContactsToCompound (
  ...., dgCollisionParamProxy& proxy) const
{
  ....
  dgCollisionInstance childInstance(*subShape, subShape->GetChildShape());
  ....
  proxy.m_referenceCollision = &childInstance; 
  ....
  m_world->CalculateConvexToConvexContacts(proxy);
  ....
}

Warning: V506 Pointer to local variable 'childInstance' is stored outside the scope of this variable. Such a pointer will become invalid. dgcollisioncompound.cpp 1815

The function receives a reference to an object of type 'dgCollisionParamProxy'. A pointer to a local variable is written to this object. The analyzer warns that this is potentially dangerous. Upon exit from the function, this pointer cannot be used, since the local variable will be destroyed.

In this case, there is no mistake. A pointer is used only when a variable exists.

I do not want to suppress such warnings with comments. The fact is that there are 9 more warnings of the same type.

We will act in another way. In all lines where a false warning is issued, a variable named 'proxy' appears. We can write one single comment that will suppress all these warnings:

// - V: proxy: 506

It needs to be entered in some file that is included in all other files. In our case, the optimal file for this is dgPhysicsStdafx.h.

Now for those lines where the word 'proxy' is found, warning V506 will not be issued. Initially, this mechanism was created to suppress warnings in macros. But, in fact, anyway, the word means a macro or something else (a variable, function, class name, etc.). The principle is simple. If the string contains the specified substring, then the corresponding warning is not displayed.

Warning N16

A long example. You can skip. You won’t lose anything interesting.

There is a vector class:

classdgVector
{
  ....
  union {
    __m128 m_type;
    __m128i m_typeInt;
    dgFloat32 m_f[4];
    struct {
      dgFloat32 m_x;
      dgFloat32 m_y;
      dgFloat32 m_z;
      dgFloat32 m_w;
    };
    struct {
      dgInt32 m_ix;
      dgInt32 m_iy;
      dgInt32 m_iz;
      dgInt32 m_iw;
    };
  };
  ....
};

And there is such code where the vector members are filled with values ​​using the memcpy () function:

DG_INLINE dgMatrix::dgMatrix (const dgFloat32* constarray)
{
  memcpy (&m_front.m_x, array, sizeof (dgMatrix)) ;
}

Warning: V512 A call of the 'memcpy' function will lead to overflow of the buffer '& m_front.m_x'. dgmatrix.h 118

The analyzer does not like that more bytes are copied to a variable of type 'dgFloat32' than it takes. Not very beautiful, but working and widely used technique. In fact, the variable m_x, m_y, m_z will fill up, and so on.

In the beginning, I was inattentive and corrected the code as follows:

memcpy(m_front.m_f, array, sizeof(dgMatrix));

I thought that only one vector is copied. And the size of the array 'm_f' just coincides with the size of the vector.

But at the next start, the analyzer pulled me again. In fact, not one vector is copied, but 4 vectors. Namely 4 vectors contains the class 'dgMatrix':

classdgMatrix
{
  ....
  dgVector m_front;
  dgVector m_up;
  dgVector m_right;
  dgVector m_posit;
  ....
}

How to fix the code so that it is beautiful and short, I do not know. Therefore, I decided to leave everything as it was and add a comment:

memcpy (&m_front.m_x, array, sizeof (dgMatrix)) ; //-V512

Warning N17, N18

void dgWorldDynamicUpdate::UpdateDynamics(dgFloat32 timestep)
{
  dgWorld* const world = (dgWorld*) this;
  dgUnsigned32 updateTime = world->m_getPerformanceCount();
  m_bodies = 0;
  m_joints = 0;
  m_islands = 0;
  m_markLru = 0;
  world->m_dynamicsLru = world->m_dynamicsLru + DG_BODY_LRU_STEP;
  m_markLru = world->m_dynamicsLru;
  ....
}

Warning: V519 The 'm_markLru' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 91, 94. dgworlddynamicupdate.cpp 94

In the variable 'm_markLru' at the beginning 0 is written, and then 'world-> m_dynamicsLru'. There is no mistake here. To get rid of the warning, I removed the variable initialization with zero.

Similarly, I acted in another place. Relevant Warning:

V519 The 'm_posit' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 1310, 1313. customvehiclecontrollermanager.cpp 1313

Warning N19, N20

dgFloat32 dgCollisionConvexPolygon::GetBoxMinRadius () const
{
  return m_faceClipSize;
}
dgFloat32 dgCollisionConvexPolygon::GetBoxMaxRadius () const
{
  return m_faceClipSize;  
}

Warning: V524 It is odd that the body of 'GetBoxMaxRadius' function is fully equivalent to the body of 'GetBoxMinRadius' function. dgcollisionconvexpolygon.cpp 88

Two functions, containing in their name 'Min' and 'Max', are arranged in the same way. For the analyzer, this is suspicious. In fact, there is no mistake. To eliminate the false positive, I implemented one function through another:

dgFloat32 dgCollisionConvexPolygon::GetBoxMaxRadius () const
{
  return GetBoxMinRadius();
}

I did the same with the GetBoxMaxRadius / GetBoxMaxRadius functions implemented in the 'dgCollisionScene' class.

Warning N21

dgInt32 AddFilterFace(dgUnsigned32 count, dgInt32* const pool){
  ....
  for (dgUnsigned32 i = 0; i < count; i ++) {
    for (dgUnsigned32 j = i + 1; j < count; j ++) {
      if (pool[j] == pool[i])
      {
        for (i = j; i < count - 1; i ++) {
          pool[i] =  pool[i + 1];
        }
        count --;
        i = count;
        reduction = true;
        break;
      }
    }
  }
  ....
}

Warning: V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 105, 108. dgpolygonsoupbuilder.cpp 108

There are two cycles. One uses the variable 'i' as the counter, the other 'j'. Inside these loops, another loop sometimes starts. It uses the variable 'i' again as a counter. The analyzer does not like this, although there is no error here.

If the inner loop is running, then the outer loops stop:

• a loop organized with the 'j' variable stops due to the 'break' statement;
• the loop organized by the variable 'i' stops due to the assignment of “i = count”.

The analyzer could not figure out such a tangle. This is an example of working code, which nevertheless smacks of smell.

To eliminate the false positive, I used a comment:

for (i = j; i < count - 1; i ++) { //-V535

Warning N22 - N25

DG_INLINE dgMatrix::dgMatrix (const dgVector& front)
{
  ....
  m_right = m_right.Scale3 (dgRsqrt (m_right % m_right));
  m_up = m_right * m_front;
  ....
}

Warning: V537 Consider reviewing the correctness of 'm_right' item's usage. dgmatrix.h 143

A V537 warning is issued when variables suspiciously mixed in the names are “right”, “left”, “front”, and so on. For this project, this diagnosis was unsuccessful. The analyzer issued 4 warnings for a completely harmless code.

In this case, in PVS-Studio you can completely disable V537 diagnostics in the settings.

CppCat cannot disable individual diagnostics. We will use an alternative approach. In all lines where there were false warnings, the word “right” is present. I added a comment to the dgStdafx.h file:

//-V:right:537

Warning N26

Note the comment.

intpthread_delay_np(struct timespec *interval){
  ....
  /*
   * Most compilers will issue a warning 'comparison always 0'
   * because the variable type is unsigned,
   * but we need to keep this
   * for some reason I can't recall now.
   */if (0 > (wait_time = secs_in_millisecs + millisecs))
  {
    return EINVAL;
  }
  ....
}

Warning: V547 Expression is always false. Unsigned type value is never <0. pthread_delay_np.c 119 The

comment tells us that this is not a mistake, but is intended. If so, we have no choice but to suppress the warning using the comment:

if (0 > (wait_time = secs_in_millisecs + millisecs)) //-V547

Warning N27

typedefunsignedlonglong dgUnsigned64;
dgUnsigned64 m_mantissa[DG_GOOGOL_SIZE];
dgGoogol::dgGoogol(dgFloat64 value)
  :m_sign(0)
  ,m_exponent(0)
{
  ....
  m_mantissa[0] = (dgInt64 (dgFloat64 (
                    dgUnsigned64(1)<<62) * mantissa));
  // it looks like GCC have problems with this
  dgAssert (m_mantissa[0] >= 0);
  ....
}

Warning: V547 Expression 'm_mantissa [0]> = 0' is always true. Unsigned type value is always> = 0. dggoogol.cpp 55

The analyzer shares the opinion of GCC that something is wrong with this code (see the comment in the code).

Checking "dgAssert (m_mantissa [0]> = 0)" does not make sense. An unsigned variable is always greater than or equal to zero. The 'dgAssert' code does not actually check anything.

Programmers are lazy. Instead of sorting out and fixing the error, they write a comment.

I fixed the code so that 'dgAssert' performs the required validation. To do this, it was necessary to create a temporary sign variable:

dgInt64 integerMantissa = (dgInt64(dgFloat64(
                             dgUnsigned64(1) << 62) * mantissa));
dgAssert(integerMantissa >= 0);
m_mantissa[0] = integerMantissa;

Warning N28 - N31

void dgRedBackNode::RemoveFixup (....) 
{
  ....
  if (!ptr) {
    return;
  }
  ....
  ptr->SetColor(RED) ;
  ptr->RotateLeft (head);
  tmp = ptr->m_right;
  if (!ptr || !tmp) {
    return;
  }
  ....
}

Warning: V560 A part of conditional expression is always false :! Ptr. dgtree.cpp 215

The expression '! ptr' is always false. The fact is that the pointer 'ptr' has already been checked for equality to zero earlier. If the pointer is zero, the function exited.

The second check looks even more stupid due to the fact that the pointer in front of it is dereferenced: "tmp = ptr-> m_right;".

I fixed the false positive by removing the second pointless check. Now the code looks like this:

if (!ptr) {
  return;
}
....
tmp = ptr->m_right;
if (!tmp) {
  return;
}
....

Similarly, I fixed 3 more code fragments.

By the way, such a code could additionally lead to V595 warnings. Specially check it out I was too lazy. If at the end of the article we do not count a couple of warnings, then the reason is precisely this.

Warning N32, N33

DG_INLINE bool dgBody::IsCollidable() const;
void dgBroadPhase::AddPair (dgBody* const body0, dgBody* const body1, const dgVector& timestep2, dgInt32 threadID)
{
  ....
  bool kinematicBodyEquilibrium =
    (((body0->IsRTTIType(dgBody::m_kinematicBodyRTTI) ?
      true : false) & body0->IsCollidable()) |
    ((body1->IsRTTIType(dgBody::m_kinematicBodyRTTI) ?
      true : false) & body1->IsCollidable())) ? false : true;
  ....
}

Warning: V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. dgbroadphase.cpp 921

Code with the "darling". It is not clear why it was necessary to write such a complex and incomprehensible test. I rewrote it. The code has become a little shorter and easier to read. Plus, the analyzer warning disappeared.

bool kinematicBodyEquilibrium =
  !((body0->IsRTTIType(dgBody::m_kinematicBodyRTTI) &&
     body0->IsCollidable()) ||
    (body1->IsRTTIType(dgBody::m_kinematicBodyRTTI) &&
     body1->IsCollidable()));

Another V564 warning was issued, where I also simplified the code:

V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. dgbroadphase.cpp 922

Warning N34 - N37

classdgAIWorld:public dgAIAgentGraph { .... };
typedefstructNewtonAIWorld{} NewtonAIWorld;
NewtonAIWorld* NewtonAICreate(){
  TRACE_FUNCTION(__FUNCTION__);
  dgMemoryAllocator* const allocator = new dgMemoryAllocator();
  NewtonAIWorld* const ai = (NewtonAIWorld*)
    new (allocator) dgAIWorld (allocator);
  return ai;
}

Warning: V572 It is odd that the object which was created using 'new' operator is immediately casted to another type. newtonai.cpp 40 A

strange way to store objects. Create an object of class 'dgAIWorld'. Explicitly cast it to type 'NewtonAIWorld'. I did not understand why this was done. Apparently, this is for some reason necessary. Just suppressed the warning using the comments in this and 3 more functions.

Warning N38

void dgCollisionCompound::EndAddRemove ()
{
  ....
  if (node->m_type == m_node) {
    list.Append(node);
  }
  if (node->m_type == m_node) {
    stack.Append(node->m_right);
    stack.Append(node->m_left);
  } 
  ....
}

Warning: V581 The conditional expressions of the 'if' operators located alongside each other are identical. Check lines: 952, 956. dgcollisioncompound.cpp 956

The analyzer does not like the fact that the same condition is checked nearby. Perhaps there is a typo here. For example, suddenly the code should be like this:

if (node->m_type == m_node) {
  ....
}
if (node->m_type == m_FOO) {
  ....
} 

In the detected code, everything is correct. To get rid of false positives, it is best to fix the code. It seems to me that I will not change the logic of the program, making only one check:

if (node->m_type == m_node) {
  list.Append(node);
  stack.Append(node->m_right);
  stack.Append(node->m_left);
}

Warning N39

void dSceneGraph::AddEdge (....)
{
  ....
  if ((!parentLink && !childLink)) {
  ....
}

Warning: V592 The expression was enclosed by parentheses twice: '((! ParentLink &&! ChildLink))'. One pair of parentheses is unnecessary or misprint is present. dscenegraph.cpp 209

It's okay. Just extra brackets. Deleted them:

if (!parentLink && !childLink) {

Warning N40 - N44

dgVector dgCollisionCylinder::SupportVertex (....) const
{
  dgAssert (dgAbsf ((dir % dir - dgFloat32 (1.0f))) <
            dgFloat32 (1.0e-3f));
  ....
}

Warning: V592 The expression was enclosed by parentheses twice: '((dir% dir - dgFloat32 (1.0f)))'. One pair of parentheses is unnecessary or misprint is present. dgcollisioncylinder.cpp 202

It's okay. Just extra brackets. Deleted them so as not to confuse the analyzer:

dgAssert (dgAbsf (dir % dir - dgFloat32 (1.0f)) <
          dgFloat32 (1.0e-3f));

This line was replicated using Copy-Paste in 4 more code fragments. There I also removed an extra pair of brackets.

Warning N45 - N65

voidptw32_throw(DWORD exception){
  ....
  ptw32_thread_t * sp =
    (ptw32_thread_t *) pthread_getspecific (ptw32_selfThreadKey);
  sp->state = PThreadStateExiting;
  if (exception != PTW32_EPS_CANCEL &&
      exception != PTW32_EPS_EXIT)
  {
    exit (1);
  }
  ....
  if (NULL == sp || sp->implicit)
  ....
}

Warning: V595 The 'sp' pointer was utilized before it was verified against nullptr. Check lines: 77, 85. ptw32_throw.c 77

Diagnostics of V595 works as follows. The analyzer considers the code suspicious if the pointer is used at the beginning and then checked for equality to zero. There are certain nuances and exceptions, but the general principle of analysis is just that.

Here is just such a case. At the beginning, the variable 'sp' is dereferenced in the expression “sp-> state”. Then it is checked for NULL.

The analyzer detected 20 more similar fragments of code. In each case, one should act differently. Somewhere I moved the check before dereferencing. Somewhere just deleted it.

Note

Very often, a false warning V595 is caused by a macro similar to this:

#define FREE(p) { if (p) free(p); }

Specifically, in this case, the analyzer must understand what the programmer had in mind and will not say anything. But in the general case, false positives can occur on such a code:

p->foo();
FREE(p);

In such cases, I recommend getting rid of macros. The FREE () macro above is completely pointless and harmful.

Firstly, it makes no sense to check the pointer to equality to zero. The free () function works correctly with null pointers. With the operator 'delete' the same thing. Therefore, the FREE () macro is not needed. Absolutely.

Secondly, it is dangerous. If we retrieve pointers from the array, this may lead to an error. Example: FREE (ArrayOfPtr [i ++]); - one pointer will be checked, and the next one will be freed.

Warning N66

void dgCollidingPairCollector::Init ()
{
  dgWorld* const world = (dgWorld*) this;
  // need to expand teh buffer is needed 
  world->m_pairMemoryBuffer[0];
  m_count = 0;
}

Warning: V607 Ownerless expression 'world-> m_pairMemoryBuffer [0]'. dgcontact.cpp 342 The

comment tells us that the expression “world-> m_pairMemoryBuffer [0]” makes sense. The analyzer does not know about this and gives a false warning. I fixed it using code markup:

world->m_pairMemoryBuffer[0]; //-V607

A more beautiful solution is to add a special method that extends the buffer. Then the code would look something like this:

void dgCollidingPairCollector::Init ()
{
  dgWorld* const world = (dgWorld*) this;
  world->m_pairMemoryBuffer.ExpandBuffer();
  m_count = 0;
}

Now a comment is not needed. The code speaks for itself. The analyzer does not generate warnings. Idyll.

Warning N67

dgGoogol dgGoogol::Floor () const
{
  ....
  dgUnsigned64 mask = (-1LL) << (64 - bits);
  ....
}

Warning: V610 Undefined behavior. Check the shift operator '<<. The left operand '(- 1LL)' is negative. dggoogol.cpp 249

You cannot shift negative numbers to the left. This leads to undefined behavior. Read more: " Without knowing the ford, do not get into the water. Part Three ."

I fixed the code as follows:

dgUnsigned64 mask = (~0LLU) << (64 - bits);

Warning N68 - N79

void dGeometryNodeSkinModifierInfo::RemoveUnusedVertices(
  constint* const vertexMap)
{
  ....
  dVector* vertexWeights = new dVector[m_vertexCount];
  dBoneWeightIndex* boneWeightIndex =
                           new dBoneWeightIndex[m_vertexCount];
  ....
  delete boneWeightIndex;
  delete vertexWeights;
}

Warnings:

• V611 The memory was allocated using 'new T []' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] boneWeightIndex;'. dgeometrynodeskinmodifierinfo.cpp 97
• V611 The memory was allocated using 'new T []' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] vertexWeights;'. dgeometrynodeskinmodifierinfo.cpp 98

The square brackets of the 'delete' operators are forgotten. These are errors that should be fixed. The correct version of the code:

delete [] boneWeightIndex;
delete [] vertexWeights;

The analyzer found 10 more such places, and everywhere I fixed the code.

Warning N80

#if defined(_MSC_VER)/* Disable MSVC 'anachronism used' warning */#pragmawarning( disable : 4229 )#endiftypedefvoid(* PTW32_CDECL ptw32_cleanup_callback_t)(void *);
#if defined(_MSC_VER)#pragmawarning( default : 4229 )#endif

Warning: V665 Possibly, the usage of '#pragma warning (default: X)' is incorrect in this context. The '#pragma warning (push / pop)' should be used instead. Check lines: 733, 739. pthread.h 739

This is a bad way to suppress warnings. Especially if this is library code. What is the reason and how to do better, you can find out from the description of the V665 diagnostic .

I fixed the code using "warning (push)" and "warning (pop)":

#if defined(_MSC_VER)/* Disable MSVC 'anachronism used' warning */#pragmawarning( push )#pragmawarning( disable : 4229 )#endiftypedefvoid(* PTW32_CDECL ptw32_cleanup_callback_t)(void *);
#if defined(_MSC_VER)#pragmawarning( pop )#endif

Warning N81 - N99

dgAABBPointTree4d* dgConvexHull4d::BuildTree (....) const
{
  ....
  const dgBigVector& p = points[i];
  ....
  varian = varian + p.CompProduct4(p);
  ....
}

Warning: V678 An object is used as an argument to its own method. Consider checking the first actual argument of the 'CompProduct4' function. dgconvexhull4d.cpp 536

The analyzer does not like function calls of the following form: X.Foo (X). Firstly, it could be a typo. Secondly, the class may not be ready for it to work with itself.

In this case, the code is correct. A false warning must be suppressed. You can put a comment of the form:

varian = varian + p.CompProduct4 (p); // - V678

This is a bad idea. The analyzer issued another 18 such warnings, and I do not want to add so many comments to the code.

Fortunately, all 19 warnings relate to calls to the CompProduct3 () or CompProduct4 functions. Therefore, you can write one comment that suppresses all V678 warnings in the lines that contain the CompProduct substring:

//-V:CompProduct:678

I posted this comment in the dgStdafx.h file.

Warning N100 - N119

The class 'dgBaseNode' contains pointers:

classdgBaseNode:public dgRef
{
  ....
  dgBaseNode (const dgBaseNode &clone);
  ....
private:
  ....
  dgBaseNode* parent;
  dgBaseNode* child;
  dgBaseNode* sibling;
};

Therefore, it implements a full-fledged copy constructor:

dgBaseNode::dgBaseNode (const dgBaseNode &clone)
  :dgRef (clone)
{
  Clear ();
  for (dgBaseNode* obj = clone.child; obj; obj = obj->sibling) {
    dgBaseNode* newObj = (dgBaseNode *)obj->CreateClone ();
    newObj->Attach (this);
    newObj->Release();
  }
}

Warning: V690 The 'dgBaseNode' class implements a copy constructor, but lacks the the '=' operator. It is dangerous to use such a class. dgnode.h 35

The Law of the Big Two is violated here . There is a copy constructor, but there is no copy operator (operator =). As a result, when assigned, the compiler will simply copy the values ​​of the pointers, which will lead to subtle errors. Even if the copy operator is not used right now, this code is potentially dangerous. It is very easy to make a mistake.

There is only one correct way of doing this - implement operator =. If this operator is not needed by the meaning, then you can declare it in the private section.

The analyzer found another 18 classes where they forgot to implement (or prohibit) the copy operator.

There is still one strange class, the meaning and purpose of which is not clear to me:

structStringPool
 {char buff[STRING_POOL_SIZE];
  StringPool ()
  {
  }
  StringPool (const StringPool &arg)
  {
  }
};

Here I just suppressed a false positive using a comment:

structStringPool //-V690
{
  ....
};

Note 1.— C ++ 11 introduced new keywords that simplify the prohibition of using copy constructors and assignment operators. Or they say that the copy constructor or assignment operator created by the default compiler is correct. It is about = default and = delete. You can read more about them, for example, in the C ++ FAQ .

Note 2. I often see that in many programs the copy or assignment operator is implemented, although it is not needed. I mean situations where an object can be perfectly copied by the compiler. A simple artificial example:

structPoint {int x, y;
  Point &Point(const Point &p){ x = p.x; y = p.y; return *this; }
};

There is an unnecessary copy operator for anyone. The rule "The Big Two Law" is violated, and the analyzer generates a warning. In order not to write another unnecessary function (copy constructor), you must remove operator =.

Great short correct class:

structPoint {int x, y;
};

Warning N120 - N125

There are 6 more warnings of various types. I did not cope with them. The project code is completely unfamiliar to me. I can’t understand if I’m dealing with an error or a false positive. Plus, even if this is a mistake, I still don’t understand how to change the code. I did not begin to torment myself and simply marked these warnings with false comments as false.

Warning N126 - N127

Two warnings are “lost." This is normal. The fact is that sometimes the same suspicious piece of code can lead to the appearance of 2, and sometimes 3 warnings. Accordingly, several warnings can be fixed with one revision. For example, V595 warnings might disappear during code editing related to V560 (see warnings N28 - N31).

findings

As you can see, there are very few false positives. Many messages indicate a code that “smells”. Yes, this code works correctly. Nevertheless, it is strange, it is difficult to read and maintain. What the analyzer can confuse can confuse the person.

Often the code snippets pointed to by the analyzer can be rewritten. This will not only eliminate the warning, but also make the code more understandable.

In the case when the analyzer is really wrong, there are various methods for suppressing false warnings. They are discussed in detail in the documentation.

I hope that I was able to show in the article that working with false positives is not as complicated as it might seem at first glance. Good luck using our static code analyzers.

This article is in English.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. Handling False Positives in PVS-Studio and CppCat .

Unfortunately, we no longer develop or support the CppCat project. You can read about the reasons here .