It occurred to me recently to reanalyze the Newton Game Dynamics physics engine. The project's code is very high-quality, so there were almost no genuine bugs detected, but I did get a few dozens of false positives. Seems like there's nothing to write about, doesn't it? Well, I thought I should write about how to handle false positives and how to avoid them. I found the Newton Game Dynamics project a good example to demonstrate that on.
Unfortunately, we are no longer developing or supporting the CppCat static code analyzer. Please read here for details.
The number of diagnostic messages the PVS-Studio analyzer generated on this project is as follows:
All of these refer to the general analysis set of diagnostic rules (GA).
So, it makes total 127 warnings that need to be examined. The CppCat analyzer generates just as many messages. Further in the article, I won't distinguish between PVS-Studio and CppCat. In general, both provide identical false positive suppression mechanisms. PVS-Studio does have a bit more of them, but it doesn't affect the whole picture much.
Note. To learn about the differences between the functional capabilities of PVS-Studio and CppCat, follow this link.
It took me three hours odd to write down all the necessary samples for the article and get rid of all the warnings. I think it would have taken me not more than an hour if I had not had to write down the examples. It suggests that the difficulty of fighting against false positives is exaggerated. It's true that they hinder and distract you; it's true that large projects contain piles of false positives. But nevertheless, it's not difficult at all to get rid of them.
The result is: CppCat generates 0 warnings; so does PVS-Studio. Well, we could turn on the third-level or 64-bit diagnostics of course, but they are not as interesting. First of all, you need to eliminate those warnings the analyzer draws your attention to as most crucial ones. And that is already a large step toward higher quality of your code. It is what you should start with. If you turn on all the diagnostics at once, you won't feel strong and patient enough to go through all of them. By the way, it's a main mistake of novice programmers. Remember that "more" doesn't mean "better".
The PVS-Studio and CppCat analyzers don't group diagnostic messages and don't sort them. You don't need this when you use them regularly: if a tool detects 2 or 3 bugs in new code, there is nothing to group and sort there, while implementing this feature would only complicate the interface.
When using the tool for the first time, you can sort messages by diagnostic number. It is done by clicking on the header of the column with diagnostic code. We decided not to implement it as an automatic feature. Warnings are displayed in the same order as files are being analyzed, which allows you to start viewing messages without having to wait for the analysis to finish. If we chose to automatically sort messages while the analysis is running, they would "jump" all over the table and you won't be able to handle them until the analysis is over.
Thus, sorting by message type (diagnostic number) is most useful at the first stages. And this is what I will do. It will allow me to quickly find false positives of one type and eliminate them, thus significantly simplifying the work and reducing the time of initial setting up.
If any of the false positive suppression methods don't seem clear enough, see the corresponding section in the documentation:
Warnings No. 1, No. 2
void
dgWorldDynamicUpdate::CalculateJointsVelocParallelKernel (....)
{
....
dgVector velocStep2 (velocStep.DotProduct4(velocStep));
dgVector omegaStep2 (omegaStep.DotProduct4(omegaStep));
dgVector test ((velocStep2 > speedFreeze2) |
(omegaStep2 > omegaStep2));
....
}Diagnostic message: V501 There are identical sub-expressions to the left and to the right of the '>' operator: omegaStep2 > omegaStep2 dgworlddynamicsparallelsolver.cpp 546
The "omegaStep2 > omegaStep2" expression looks suspicious. I cannot say for sure if there is a genuine error here or not. Because this comparison can also be found in another file, I guess it's not a bug but the programmer's conscious intention.
Let's assume it's not an error. I have marked these two fragments with a special comment:
dgVector test ((velocStep2 > speedFreeze2) |
(omegaStep2 > omegaStep2)); //-V501From now on, the V501 warning will not be generated for these fragments.
Warning No. 3
dgInt32
dgWorld::CalculatePolySoupToHullContactsDescrete(....) const
{
....
dgAssert (dgAbsf(polygon.m_normal % polygon.m_normal -
dgFloat32 (1.0f)) < dgFloat32 (1.0e-4f));
....
}Diagnostic message: 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 is both right and wrong about this code fragment. On the one hand, the "polygon.m_normal % polygon.m_normal" expression is very suspicious indeed. On the other hand, the analyzer just can't figure out that this is a test to check the '%' operator implemented in a class. So the code is actually correct. Let's help the analyzer by adding a comment:
dgAssert (dgAbsf(polygon.m_normal % polygon.m_normal - //-V501
dgFloat32 (1.0f)) < dgFloat32 (1.0e-4f));Warning No. 4
static void PopupateTextureCacheNode (dScene* const scene)
{
....
if (!(info->IsType(dSceneCacheInfo::GetRttiType()) ||
info->IsType(dSceneCacheInfo::GetRttiType()))) {
....
}Diagnostic message: V501 There are identical sub-expressions 'info->IsType(dSceneCacheInfo::GetRttiType())' to the left and to the right of the '||' operator. dscene.cpp 125
One and the same condition is checked twice. Suppose the second check is redundant. To get rid of the false warning, I fixed the code in the following way:
if (!(info->IsType(dSceneCacheInfo::GetRttiType()))) {Warning No. 5
dFloat dScene::RayCast (....) const
{
....
dFloat den = 1.0f /
((globalP1 - globalP0) % (globalP1 - globalP0)); //-V501
....
}Diagnostic message: 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 'dVector' class. Because of that, this code makes sense and the analyzer's worry is all for nothing. Let's mark the code with the comment:
dFloat den = 1.0f /
((globalP1 - globalP0) % (globalP1 - globalP0)); //-V501Although the analyzer is wrong, this code still can't be called neat. I suggest implementing special functions or something else for such cases.
Warnings No. 6 - No. 15
dgInt32 dgCollisionCompound::CalculateContactsToCompound (
...., dgCollisionParamProxy& proxy) const
{
....
dgCollisionInstance childInstance
(*subShape, subShape->GetChildShape());
....
proxy.m_referenceCollision = &childInstance;
....
m_world->CalculateConvexToConvexContacts(proxy);
....
}Diagnostic message: 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 the 'dgCollisionParamProxy' type. A pointer to a local variable is written into this object, and the analyzer warns that it is potentially dangerous. After leaving the function, this pointer can't be used because the local variable it points to will be destroyed by then.
There is no error in this particular case. The pointer is used only while the variable exists.
I don't feel like using comments to suppress such warnings. You see, there are 9 more of them, all of the same kind.
So let's do it another way. All the lines these false positives are generated on contain a variable named 'proxy'. We can write one single comment to suppress all these warnings at once:
//-V:proxy:506
It should be added into some file that gets included into all the other files. In our case, the file "dgPhysicsStdafx.h" will do best.
From now on, the V506 warning won't be displayed for any lines containing the word 'proxy'. This mechanism was initially implemented to suppress warnings in macros. But it doesn't actually matter if a word serves as a name for a macro or some other entity (a variable, function, class, etc.). The principle behind it is simple: if a string contains a specified substring, the corresponding warning is not displayed.
Warning No. 16
The following sample is a lengthy one. There's nothing of much interest about it, so you may skip it.
We have a vector class:
class dgVector
{
....
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 we have the following piece of code where vector members are filled with values by the memcpy() function:
DG_INLINE dgMatrix::dgMatrix (const dgFloat32* const array)
{
memcpy (&m_front.m_x, array, sizeof (dgMatrix)) ;
}Diagnostic message: V512 A call of the 'memcpy' function will lead to overflow of the buffer '& m_front.m_x'. dgmatrix.h 118
The analyzer doesn't like that more bytes are written into the variable of the 'dgFloat32' type than it actually occupies. Far from neat, this practice, however, works well and is widely used. The function is actually filling with values the variables m_x, m_y, m_z, and so on.
I was not attentive enough for the first time and just fixed the code in the following way:
memcpy(m_front.m_f, array, sizeof(dgMatrix));I thought that only one vector was copied, while the size of the 'm_f' array is just the same as that of the vector.
But at the next launch, the analyzer drew my attention to the code once again. There were actually 4 vectors to be copied, not one. And it is 4 vectors that the 'dgMatrix' class contains:
class dgMatrix
{
....
dgVector m_front;
dgVector m_up;
dgVector m_right;
dgVector m_posit;
....
}I don't know how to make this code neat and short, so I decided to leave it all as it had been before and just added the comment:
memcpy (&m_front.m_x, array, sizeof (dgMatrix)) ; //-V512Warnings No. 17, No. 18
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;
....
}Diagnostic message: V519 The 'm_markLru' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 91, 94. dgworlddynamicupdate.cpp 94
The 'm_markLru' variable is first initialized by 0 and then 'world->m_dynamicsLru' is written into it. There is no error here. To get rid of the warning, I removed the variable initialization by zero.
Just in the same way I fixed one more code fragment. The corresponding diagnostic message:
V519 The 'm_posit' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1310, 1313. customvehiclecontrollermanager.cpp 1313
Warnings No. 19, No. 20
dgFloat32 dgCollisionConvexPolygon::GetBoxMinRadius () const
{
return m_faceClipSize;
}
dgFloat32 dgCollisionConvexPolygon::GetBoxMaxRadius () const
{
return m_faceClipSize;
}Diagnostic message: V524 It is odd that the body of 'GetBoxMaxRadius' function is fully equivalent to the body of 'GetBoxMinRadius' function. dgcollisionconvexpolygon.cpp 88
Two functions whose names contain the words 'Min' and 'Max' are implemented in the same way. The analyzer finds it suspicious. But everything is OK here. To eliminate the false positive, I implemented one function through the other:
dgFloat32 dgCollisionConvexPolygon::GetBoxMaxRadius () const
{
return GetBoxMinRadius();
}In the same way I handled the functions GetBoxMaxRadius/GetBoxMaxRadius implemented in the 'dgCollisionScene' class.
Warning No. 21
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;
}
}
}
....
}Diagnostic message: V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 105, 108. dgpolygonsoupbuilder.cpp 108
We have two loops here. One of them uses the variable 'i' as a counter; the other, the variable 'j'. Inside these loops, one more loop is sometimes run. It uses the variable 'i' as a counter, too. The analyzer doesn't like it, though there is no error here.
When an internal loop is executed, external loops are stopped:
The analyzer failed to figure out these nuances. The fragment above is a fine example of code that works but smells.
I used commenting to eliminate the false positive:
for (i = j; i < count - 1; i ++) { //-V535Warnings No. 22 - No. 25
DG_INLINE dgMatrix::dgMatrix (const dgVector& front)
{
....
m_right = m_right.Scale3 (dgRsqrt (m_right % m_right));
m_up = m_right * m_front;
....
}Diagnostic message: V537 Consider reviewing the correctness of 'm_right' item's usage. dgmatrix.h 143
The analyzer generates the V537 warning when it comes across a suspicious mixture of variables whose names contain the words "right", "left", "front", and the like. This diagnostic proved unsuccessful for this project as the analyzer generated 4 warnings on absolutely safe code.
In that case, we can completely turn this diagnostic off in PVS-Studio's settings.
In CppCat, you can't turn off single diagnostics, so we have to use an alternative method. All the lines that triggered the false positives contain the word "right". I added the following comment into the file "dgStdafx.h":
//-V:right:537Warning No. 26
Notice the comment.
int pthread_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;
}
....
}Diagnostic message: V547 Expression is always false. Unsigned type value is never < 0. pthread_delay_np.c 119
The comment tells us that it's not a bug, but the programmer's conscious intention. Well, in that case we only have to suppress the warning by a comment:
if (0 > (wait_time = secs_in_millisecs + millisecs)) //-V547Warning No. 27
typedef unsigned long long 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);
....
}Diagnostic message: V547 Expression 'm_mantissa[0] >= 0' is always true. Unsigned type value is always >= 0. dggoogol.cpp 55
The analyzer shares GCC's opinion that something is wrong with this code (see the comment in the code).
The check "dgAssert(m_mantissa[0] >= 0)" makes no sense: an unsigned variable is always equal to or larger than zero. 'dgAssert' doesn't actually check anything.
Programmers tend to be lazy. They would rather write a comment than spend some time to investigate an issue and fix a mistake.
I fixed the code so that 'dgAssert' executed a correct check. For this purpose, I had to add a temporary signed variable:
dgInt64 integerMantissa = (dgInt64(dgFloat64(
dgUnsigned64(1) << 62) * mantissa));
dgAssert(integerMantissa >= 0);
m_mantissa[0] = integerMantissa;Warnings No. 28 - No. 31
void dgRedBackNode::RemoveFixup (....)
{
....
if (!ptr) {
return;
}
....
ptr->SetColor(RED) ;
ptr->RotateLeft (head);
tmp = ptr->m_right;
if (!ptr || !tmp) {
return;
}
....
}Diagnostic message: V560 A part of conditional expression is always false: !ptr. dgtree.cpp 215
The '!ptr' expression is always false. The reason is that the 'ptr' pointer has already been checked for being null before. If it was found to be null, the function would be left.
The second check looks even sillier because of the pointer being dereferenced before it: "tmp = ptr->m_right;".
I eliminated the false positive by removing the second meaningless check. The code now looks like this:
if (!ptr) {
return;
}
....
tmp = ptr->m_right;
if (!tmp) {
return;
}
....In the same way I fixed 3 other code fragments.
By the way, this code could additionally trigger the V595 warning. I felt too lazy to check it for sure, but if we miss a couple of warnings in the end of the article, be aware that it will be just because of that.
Warnings No. 32, No. 33
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;
....
}Diagnostic message: 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
This code smells. I don't think I understand why the programmer would need such a complicated and obscure check. I rewrote it, and the code became a bit shorter and more readable. Besides, I got rid of the false warning.
bool kinematicBodyEquilibrium =
!((body0->IsRTTIType(dgBody::m_kinematicBodyRTTI) &&
body0->IsCollidable()) ||
(body1->IsRTTIType(dgBody::m_kinematicBodyRTTI) &&
body1->IsCollidable()));There was one more V564 warning, and I simplified the corresponding code fragment too:
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
Warnings No. 34 - No. 37
class dgAIWorld: public dgAIAgentGraph { .... };
typedef struct NewtonAIWorld{} NewtonAIWorld;
NewtonAIWorld* NewtonAICreate ()
{
TRACE_FUNCTION(__FUNCTION__);
dgMemoryAllocator* const allocator = new dgMemoryAllocator();
NewtonAIWorld* const ai = (NewtonAIWorld*)
new (allocator) dgAIWorld (allocator);
return ai;
}Diagnostic message: V572 It is odd that the object which was created using 'new' operator is immediately casted to another type. newtonai.cpp 40
That's a pretty strange way to store objects: creating an object of the 'dgAIWorld' class and casting it explicitly to the ' NewtonAIWorld' type. I didn't feel like figuring out why it had been done - there must have been some reason; I simply suppressed this warning by a comment in this and 3 other functions.
Warning No. 38
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);
}
....
}Diagnostic message: V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 952, 956. dgcollisioncompound.cpp 956
The analyzer doesn't like one and the same condition being checked twice on end. Perhaps there is some typo here. What if this code was meant to look like this:
if (node->m_type == m_node) {
....
}
if (node->m_type == m_FOO) {
....
}However, that code sample is alright. To get rid of the false positive, we should fix the code. I don't think I will violate the program execution logic by leaving only one check:
if (node->m_type == m_node) {
list.Append(node);
stack.Append(node->m_right);
stack.Append(node->m_left);
}Warning No. 39
void dSceneGraph::AddEdge (....)
{
....
if ((!parentLink && !childLink)) {
....
}Diagnostic message: 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 OK, just redundant parentheses. I removed them:
if (!parentLink && !childLink) {Warnings No. 40 - No. 44
dgVector dgCollisionCylinder::SupportVertex (....) const
{
dgAssert (dgAbsf ((dir % dir - dgFloat32 (1.0f))) <
dgFloat32 (1.0e-3f));
....
}Diagnostic message: 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 alright, just redundant parentheses. I removed them so that the analyzer didn't worry:
dgAssert (dgAbsf (dir % dir - dgFloat32 (1.0f)) <
dgFloat32 (1.0e-3f));This line was replicated to 4 other code fragments through the Copy-Paste method. I fixed those too.
Warnings No. 45 - No. 65
void
ptw32_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)
....
}Diagnostic message: V595 The 'sp' pointer was utilized before it was verified against nullptr. Check lines: 77, 85. ptw32_throw.c 77
The V595 diagnostic works in the following way. The analyzer considers the code suspicious if a pointer is first dereferenced and then checked for being null. There are certain nuances and exceptions for rule, but the general principle is just like I said.
Here we have just such a case: the 'sp' variable is first dereferenced in the expression "sp->state" and then is checked for being null.
The analyzer has detected 20 more fragments like that. In each particular case, we should act differently: in some fragments I placed the check before the dereferencing operation and in some other fragments I simply removed it.
Note
False V595 warnings are very often triggered by macros of the following pattern:
#define FREE(p) { if (p) free(p); }In this particular case, the analyzer will figure out the programmer's intention and keep silent. But in general, the following code pattern may trigger false positives:
p->foo();
FREE(p);In these cases, I recommend that you throw macros away completely. The FREE() macro shown above is absolutely meaningless and even harmful.
Firstly, you don't have to check the pointer for being null. The free() function handles null pointers correctly. The same is true for the 'delete' operator. That's why the FREE() macro is not needed - at all.
Secondly, it is dangerous. If we extract pointers from an array, it may cause an error. For example: FREE(ArrayOfPtr[i++]) - the first pointer will be checked, and the next one will be freed.
Warning No. 66
void dgCollidingPairCollector::Init ()
{
dgWorld* const world = (dgWorld*) this;
// need to expand teh buffer is needed
world->m_pairMemoryBuffer[0];
m_count = 0;
}Diagnostic message: V607 Ownerless expression 'world->m_pairMemoryBuffer[0]'. dgcontact.cpp 342
The comment tells us that the "world->m_pairMemoryBuffer[0]" expression makes sense. The analyzer, however, doesn't know that and generates a false positive. I removed it by adding a comment:
world->m_pairMemoryBuffer[0]; //-V607A nicer solution would be to add a special method expanding 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;
}We don't need the comment anymore - the code says it all by itself. The analyzer doesn't generate any warnings, and everything's fine.
Warning No. 67
dgGoogol dgGoogol::Floor () const
{
....
dgUnsigned64 mask = (-1LL) << (64 - bits);
....
}Diagnostic message: V610 Undefined behavior. Check the shift operator '<<. The left operand '(- 1LL)' is negative. dggoogol.cpp 249
You cannot shift negative numbers to the left - it leads to undefined behavior. To learn more about that, see the article "Wade not in unknown waters. Part three".
I fixed the code in the following way:
dgUnsigned64 mask = (~0LLU) << (64 - bits);Warnings No. 68 - No. 79
void dGeometryNodeSkinModifierInfo::RemoveUnusedVertices(
const int* const vertexMap)
{
....
dVector* vertexWeights = new dVector[m_vertexCount];
dBoneWeightIndex* boneWeightIndex =
new dBoneWeightIndex[m_vertexCount];
....
delete boneWeightIndex;
delete vertexWeights;
}Diagnostic messages:
Square brackets are missing near the 'delete' operators. It is a mistake and it must be fixed. The correct code will look as follows:
delete [] boneWeightIndex;
delete [] vertexWeights;The analyzer found 10 more fragments like that, and I fixed them all.
Warning No. 80
#if defined(_MSC_VER)
/* Disable MSVC 'anachronism used' warning */
#pragma warning( disable : 4229 )
#endif
typedef void (* PTW32_CDECL ptw32_cleanup_callback_t)(void *);
#if defined(_MSC_VER)
#pragma warning( default : 4229 )
#endifDiagnostic message: 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 code refers to the library. To find out the reason why and how to fix this code, see the description of the V665 diagnostic.
I fixed the code by using "warning(push)" and " warning(pop)":
#if defined(_MSC_VER)
/* Disable MSVC 'anachronism used' warning */
#pragma warning( push )
#pragma warning( disable : 4229 )
#endif
typedef void (* PTW32_CDECL ptw32_cleanup_callback_t)(void *);
#if defined(_MSC_VER)
#pragma warning( pop )
#endifWarnings No. 81 - No. 99
dgAABBPointTree4d* dgConvexHull4d::BuildTree (....) const
{
....
const dgBigVector& p = points[i];
....
varian = varian + p.CompProduct4(p);
....
}Diagnostic message: 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 doesn't like the calls of the X.Foo(X) pattern. Firstly, it may be a typo. Secondly, the class may not be ready for handling itself.
In this particular case, the code is correct. The false positive should be suppressed. We could use the following comment, for example:
varian = varian + p.CompProduct4(p); //-V678
But that's a bad idea. The analyzer has generated 18 more warnings of this kind, and you don't want to add so many comments into the code.
Fortunately, all the 19 warnings refer to calls of the functions CompProduct3() or CompProduct4. So you can write only one comment to suppress all the V678 warnings in lines containing the substring "CompProduct":
//-V:CompProduct:678I placed this comment in the file dgStdafx.h.
Warnings No. 100 - No. 119
The 'dgBaseNode' class contains pointers:
class dgBaseNode: public dgRef
{
....
dgBaseNode (const dgBaseNode &clone);
....
private:
....
dgBaseNode* parent;
dgBaseNode* child;
dgBaseNode* sibling;
};Because of that, it has a full-blown 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();
}
}Diagnostic message: 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: the copy constructor is present, but the copy assignment operator = is missing. It will result in the compiler simply copying pointers' values while executing assignment, which will in its turn give birth to hard-to-find bugs. Even if the = operator is not used currently, this code is potentially dangerous as you may very easily make a mistake.
There is only one correct way to fix it all - implement the = operator. If this operator is not needed according to the code's logic, you can declare it private.
The analyzer has found 18 more classes with the = operator missing (or not forbidden).
There is also one strange class whose meaning and purpose I failed to figure out:
struct StringPool
{
char buff[STRING_POOL_SIZE];
StringPool ()
{
}
StringPool (const StringPool &arg)
{
}
};I simply suppressed the false positive by a comment:
struct StringPool //-V690
{
....
};Note 1. C++11 has new keywords to make it simpler to forbid the use of copy constructors and copy assignment operators; or to tell the compiler that the copy constructor or = operator created by the compiler by default are correct. What I mean are =default and =delete.
Note 2. In many programs, copy assignment operators or assignment operators are implemented, though they are not needed. I mean the situation when an object can be easily copied by the compiler. Here is a simple artificial example:
struct Point {
int x, y;
Point &Point(const Point &p) { x = p.x; y = p.y; return *this; }
};This code contains a = operator that no one needs. The "Law of the Big Two" is violated here, and the analyzer generates the warning. To avoid writing one more unnecessary function (the copy constructor), we need to delete the = operator.
Here is an excellent short and correct class:
struct Point {
int x, y;
};Warnings No. 120 - No. 125
We have 6 more warnings of different types left. I failed to cope with them as I am absolutely unfamiliar with the code. I can't figure out if I'm dealing with a genuine bug or a false positive. Besides, even if it is an error, I still don't see how to fix it. I didn't feel like worrying my head off about it and simply marked them as false positives.
Warnings No. 126 - No. 127
Two warnings have been "lost". It's OK. You see, one and the same suspicious code fragment may sometimes trigger 2 or even 3 warnings. Therefore, one fix can eliminate several warnings at once. For example, V595 warnings might have disappeared because of the fixes related to the V560 diagnostic (see warnings No. 28 - No. 31).
As you can see, there are pretty few false positives as such. Most warnings point out smelling code fragments. They work indeed, but they are still pretty strange, hard to read and maintain. What can confuse the analyzer is even more likely to confuse a human.
Many of the code fragments the analyzer didn't like can be rewritten. It will not only help to eliminate a warning, but also make the code clearer.
For those cases when the analyzer is obviously wrong, it provides you with a number of various methods of false positive suppression. They are described in detail in the documentation.
I hope I have managed to demonstrate in this article that handling false positives is far not as difficult as it might seem at first. I wish you luck in mastering our static code analyzers.