Working with false positives in PVS-Studio and CppCat
Recently, I decided to re-check the physics engine of Newton Game Dynamics. Project code quality. Therefore, there were almost no warnings that detected errors. But there were several dozen false positives. It seems to write an article about. But I got the idea that you can write about how to work with false positives, and how to make them out. The Newton Game Dynamics project seemed to me a suitable candidate.
Project Verification
The PVS-Studio analyzer generates the following number of warnings:
|
Total 127 warnings recommended for viewing. Exactly the same number of messages is issued by the CppCat analyzer. Further in the article, no distinction is made between PVS-Studio and CppCat analyzers. In general, they provide the same mechanisms for suppressing false positives. There are a little more of them in PVS-Studio, 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 more than three hours to write out all the necessary examples for the article and get rid of all the warnings. I think if I did not write out the examples, I would have managed in an hour. So the difficulty of dealing with false positives is exaggerated. Yes, they interfere and distract. Yes, if the project is large, then there are many false warnings. Nevertheless, it is not difficult to get rid of them.
Total. CppCat issues 0 warnings. PVS-Studio also produces nothing. You can, of course, enable level 3 or 64-bit diagnostics, but this is not so interesting. Getting started is very good at getting rid of recommended warnings. This is a very big step towards improving the quality of the code. This is the place to start. If you immediately include all warnings, you do not have enough strength to immediately reach the end. This is, by the way, the main mistake of newbies. “More” does not mean “better.”
View Report
PVS-Studio and CppCat analyzers do not combine diagnostic messages into groups and do not sort them. With regular use it is not necessary. If the analyzer detects 2-3 errors in the new code, there is nothing to merge and sort. Only an extra complication of the interface.
When you first start working with the tool, you can sort the messages by diagnostic number. This is done by clicking the mouse on the header of the diagnostic number column. We didn’t do this sorting automatically. Warnings are displayed in the order of file analysis. This allows, without waiting for the analysis to stop, to start watching messages. If they are sorted, then while the check is in progress, the messages in the table will “jump” and it will be impossible to work with them.
So, in the initial stages, sorting by alert type (diagnostics number) will be useful. I'll do it. This will allow me to quickly identify the same type of false positives and eliminate them. This can significantly simplify the work and reduce the initial setup time.
Work with warnings
If any of the ways to suppress false positives does not seem clear enough, then we suggest that you familiarize yourself with the relevant section of the documentation:
- For PVS-Studio - Suppression of false warnings .
- For CppCat - the documentation comes as part of the distribution.
Warning N1, N2
void dgWorldDynamicUpdate::CalculateJointsVelocParallelKernel (....) { .... dgVector velocStep2 (velocStep.DotProduct4(velocStep)); dgVector omegaStep2 (omegaStep.DotProduct4(omegaStep)); dgVector test ((velocStep2 > speedFreeze2) | (omegaStep2 > omegaStep2)); .... } Warning: V501 operator: omegaStep2> omegaStep2 dgworlddynamicsparallelsolver.cpp 546
The expression "omegaStep2> omegaStep2" looks suspicious. I find it difficult to judge whether there is a mistake or not. Since this comparison is found in another file, then, probably, this is still not an error, but also conceived.
Let there be no mistake. I tagged these two places with a comment:
dgVector test ((velocStep2 > speedFreeze2) | (omegaStep2 > omegaStep2)); //-V501 Now, in this place the 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 operator: polygon.m_normal% polygon.m_normal dgnarrowphasecollision.cpp 1921
The analyzer 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. Let's help the analyzer with a comment:
dgAssert (dgAbsf(polygon.m_normal % polygon.m_normal - //-V501 dgFloat32 (1.0f)) < dgFloat32 (1.0e-4f)); Warning n4
static void PopupateTextureCacheNode (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 | operator. dscene.cpp 125
The same is checked twice. We assume that the second check is superfluous. Therefore, I corrected 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) "%" 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. 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 features 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 of scope. Such a pointer will become invalid. dgcollisioncompound.cpp 1815
The function comes with a link to an object of the type 'dgCollisionParamProxy'. A pointer to a local variable is written to this object. The analyzer warns that it is potentially dangerous. Upon exiting the function, this pointer will not be used, since the local variable will be destroyed.
In this case, there is no error. The pointer is used only when the variable exists.
Comment suppress such warnings do not want. The fact is that there are 9 more similar warnings.
We proceed in another way. In all lines where a false warning is issued, a variable with the name 'proxy' appears. We can write one single comment that suppresses all these warnings:
// - V: proxy: 506
It needs to be entered into some file that is included in all other files. In our case, the optimal for this is the file "dgPhysicsStdafx.h".
Now for those lines where the word 'proxy' occurs, the warning V506 will not be issued. Initially, this mechanism was created to suppress warnings in macros. But, in fact, all the same, the word means a macro or something else (a variable, a function, a class name, etc.). The principle is simple. If the string contains the specified substring, the corresponding warning is not displayed.
Warning n16
Long example. You can skip it. Nothing interesting to lose.
There is 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 there is such a code where vector members are populated with values ​​using the memcpy () function:
DG_INLINE dgMatrix::dgMatrix (const dgFloat32* const array) { memcpy (&m_front.m_x, array, sizeof (dgMatrix)) ; } Warning: V512 A call of the memcpy function will lead to the overflow of the buffer. dgmatrix.h 118
The analyzer does not like that more bytes are copied into variables of the 'dgFloat32' type than it takes. Not very beautiful, but working and widely used technique. In fact, the variable m_x, m_y, m_z and so on will be filled.
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 was copied. And the size of the array 'm_f' just coincides with the size of the vector.
But at the next launch, the analyzer pulled me back. In fact, not one vector is copied, but 4 vectors. Exactly 4 vectors contains the class 'dgMatrix':
class dgMatrix { .... 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. So 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; .... } Caution: 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 error here. To get rid of the warning, I removed the initialization of the variable to zero.
Similarly, I entered another place. Corresponding 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: It is the odd that the body of the GetBoxMaxRadius function is fully equivalent to the body of the GetBoxMinRadius function. dgcollisionconvexpolygon.cpp 88
Two functions, containing in their names 'Min' and 'Max', are arranged in the same way. For the analyzer it is suspicious. Actually, there is no error. To eliminate false positives, 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. Check lines: 105, 108. dgpolygonsoupbuilder.cpp 108
There are two cycles. One uses the variable 'i' as the counter, the other 'j'. Inside these cycles, another cycle is sometimes started. As a counter, he again uses the variable 'i'. The analyzer does not like it, although there is no error here.
If an inner loop is executed, then the outer loop stops:
- a loop organized with the variable 'j' is stopped due to the operator 'break';
- the loop organized with the variable 'i' is stopped due to the assignment “i = count”.
To eliminate the false positive, I used the 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
The V537 warning is displayed when variables are suspiciously mixed with names that have “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 the V537 diagnostics in the settings.
CppCat cannot disable individual diagnostics. We 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
Pay attention to 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; } .... } 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 it was conceived. 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
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); .... } 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 there is something 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 existing dgAssert code doesn’t actually check anything.
Programmers are lazy. Instead of sorting out and correcting the error, they write a comment.
I corrected the code so that 'dgAssert' performs the necessary checks. To do this, it was necessary to create a temporary signed 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 'ptr' pointer has already been checked for equality to zero. 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 eliminated the false positive by removing the second meaningless check. Now the code looks like this:
if (!ptr) { return; } .... tmp = ptr->m_right; if (!tmp) { return; } .... Similarly corrected 3 more code fragments.
By the way, such code could additionally lead to V595 warnings. Specially check it out, I was too lazy. If at the end of the article we’ve missed a couple of warnings, the reason is just that.
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 the operator. dgbroadphase.cpp 921
The code with the "smell". It is not clear why it was necessary to write such a complex and incomprehensible check. I rewrote it. The code has become a bit shorter and easier to read. Plus lost analyzer warning.
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 the operator. dgbroadphase.cpp 922
Warning N34 - N37
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; } Warning: This is what the object was created when it was casted to another type. newtonai.cpp 40
Strange way to store objects. Create an object of class 'dgAIWorld'. Explicitly converting it to the 'NewtonAIWorld' type. I did not understand why this was done. Apparently, this is for something needed. Just suppressed the warning with comments in this and another 3 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' are located alongside each other are identical. Check lines: 952, 956. dgcollisioncompound.cpp 956
The analyzer does not like that the same condition is checked next to it. Perhaps there is a typo. 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 the false positives, it is best to fix the code. As it seems to me, 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
Nothing wrong. Just extra brackets. Removed 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
Nothing wrong. Just extra brackets. Removed them so as not to embarrass the analyzer:
dgAssert (dgAbsf (dir % dir - dgFloat32 (1.0f)) < dgFloat32 (1.0e-3f)); This line is replicated using Copy-Paste in 4 more code fragments. There I also removed an extra pair of brackets.
Warning N45 - N65
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) .... } Warning: V595 Check lines: 77, 85. ptw32_throw.c 77
Diagnostics V595 works as follows. The analyzer considers the code to be suspicious if the pointer is first used 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". It is then checked for NULL equality.
The analyzer has detected 20 more similar code fragments. In each case, you should do differently. Somewhere I moved the check to dereference. Somewhere just deleted it.
Note
Very often the cause of a false warning V595 is 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 keep silent. But in general, false positives may occur on this code:
p->foo(); FREE(p); In such cases, I recommend getting rid of macros. The above FREE () macro is completely meaningless and harmful.
First, it makes no sense to check the pointer for equality to zero. The free () function works correctly with null pointers. The delete operator is the same. Therefore, the FREE () macro is not needed. Totally.
Secondly, it is dangerous. If we retrieve pointers from an array, this can lead to an error. Example: FREE (ArrayOfPtr [i ++]); - one pointer will be checked and the next one released.
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 it and gives a false warning. I eliminated it using code markup:
world->m_pairMemoryBuffer[0]; //-V607 A more beautiful solution is to add a special method that expands 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 the comment is not needed. The code speaks for itself. The analyzer does not issue 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 move negative numbers to the left. This leads to undefined behavior. More: " Without knowing the ford, do not climb into the water. Part three ."
I corrected the code as follows:
dgUnsigned64 mask = (~0LLU) << (64 - bits); Warning N68 - N79
void dGeometryNodeSkinModifierInfo::RemoveUnusedVertices( const int* 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 the 'new T []' operator. Consider inspecting this code. It's probably better to use 'delete [] boneWeightIndex;'. dgeometrynodeskinmodifierinfo.cpp 97
- V611 The memory was allocated using the 'new T []' operator. Consider inspecting this code. It's probably better to use 'delete [] vertexWeights;'. dgeometrynodeskinmodifierinfo.cpp 98
delete [] boneWeightIndex; delete [] vertexWeights; The analyzer found 10 more such places, and everywhere I corrected the code.
Warning n80
#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 ) #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 it is library code. What is the reason and how to do better, you can learn from the description of the diagnostic V665 .
I fixed the code 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 ) #endif Warning N81 - N99
dgAABBPointTree4d* dgConvexHull4d::BuildTree (....) const { .... const dgBigVector& p = points[i]; .... varian = varian + p.CompProduct4(p); .... } Alert: V678 Consider comproduct4 function. dgconvexhull4d.cpp 536
The analyzer does not like calls of the following functions: X.Foo (X). First, it may be a typo. Secondly, the class may not be ready for the fact that he will have to work with himself.
In this case, the code is correct. A false warning needs to be suppressed. You can put a comment like:
varian = varian + p. CompProduct4 (p); // - V678
This is a bad idea. The analyzer issued 18 more such warnings, and I don’t want to add so many comments to the code.
Fortunately, all 19 warnings are related to CompProduct3 () or CompProduct4 function calls. Therefore, you can write one comment that suppresses all V678 warnings in the lines containing the substring "CompProduct":
//-V:CompProduct:678 I posted this comment in the dgStdafx.h file.
Warning N100 - N119
The class 'dgBaseNode' contains pointers:
class dgBaseNode: 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 not the the '=' operator. It is dangerous to use such a class. dgnode.h 35
Here is broken the " Law of the Big Two ". There is a copy constructor, but there is no copy operator (operator =). As a result, when assigning, the compiler will simply copy the values ​​of the pointers, which will lead to subtle errors. Even if now the copy operator is not used, this code is potentially dangerous. It is very easy to make a mistake.
The correct option is one here - implement operator =. If this operator is not needed, then you can declare it in the private section.
The analyzer found 18 more classes where they forgot to implement (or deny) the copy operator.
There is still one strange class, the meaning and purpose of which is not clear to me:
struct StringPool { char buff[STRING_POOL_SIZE]; StringPool () { } StringPool (const StringPool &arg) { } }; Here I just suppressed the false positive with a comment:
struct StringPool //-V690 { .... }; Note 1. In C ++ 11, new keywords have appeared that simplify the prohibition of using copy constructors, assignment operators. Or they say that the copy constructor or the assignment statement created by the default compiler are correct. We are talking about = default and = delete. You can read more about them, for example, in the C ++ FAQ .
Note 2. I often see that many programs implement a copy or assignment operator, although it is not needed. I mean the situation when the object can be perfectly copied by the compiler. A simple artificial example:
struct Point { int x, y; Point &Point(const Point &p) { x = px; y = py; return *this; } }; There is no unnecessary copy operator. The rule of the Big Two Law is violated, and the analyzer issues a warning. In order not to write another unnecessary function (copy constructor), you need to delete operator =.
Excellent short correct class:
struct Point { int x, y; }; Warning N120 - N125
There are still 6 different warnings left . I did not cope with them. The project code is completely unfamiliar to me. I can not understand, I am 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 didn’t torture myself and just marked these warnings with the help of comments as false.
Warning N126 - N127
Two warnings are "lost." This is normal.The fact is that sometimes the same suspicious code snippet can lead to 2 and sometimes 3 warnings. Accordingly, several edits can be corrected with a single edit. For example, V595 warnings could disappear during the code editing process associated with the V560 (see warnings N28 - N31).
findings
As you can see, there are very few false positives. Many messages point to 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 a person.
Often, the code fragments 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 of suppressing false warnings. They are discussed in detail in the documentation.
I hope I managed to show in the article that working with false positives is not as difficult as it may 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 .
Read the article and have a question?
. : PVS-Studio CppCat, 2014 . , .
| Unfortunately, we no longer develop or support the CppCat project. You can read about the reasons here . |
Source: https://habr.com/ru/post/227019/
All Articles