How I Improved My Legacy C++ Project with PVS-Studio

Since a few months, I’ve been refactoring my old C++/OpenGL project. Thus far, I used compilers (MSVC and Clang), my knowledge or free tools. At some point, I also got a chance to leverage a solid static analysis tool - PVS-Studio. The tool helped me with identifying 8 critical issues not to mention good code style and performance enhancements (in total 137 warnings)

Read on to see my report.

Starting with PVS-Studio  

I’m working on a project which is a visualisation of various sorting algorithms, written in Win32Api, C++, OpenGL. I always put a nice GIF that presents how it works:

You can read my previous articles that describe the project in detail:

• “Use the Force, Luke”… or Modern C++ Tools
• 6 Efficient Things You Can Do to Refactor a C++ Project

After doing some basic refactoring, using some modern features and even checking code with C++ Core Guideline Checkers (available in Visual Studio) I also run a professional static analysis tool: PVS Studio - I used the latest version: PVS-Studio 7.09 (August 27, 2020)

Running the analyser is very simple. Inside Visual Studio 2019 you have to select:

Extensions->PVS-Studio->Check->Solution

This action starts the PVS process which can last a dozen of seconds (for small projects) or a couple of minutes… or longer - depending on your project size.

After the check completes, you can see the following window with all of the messages:

This shows all issues that the tool has found for the solution (You can also check a single project or a single compilation unit).

As you can see, the numbers are not large, because my project is relatively small (5kloc), yet it helped me with improving the code in several places.

What I like about PVS-Studio is its super handy UI: it’s just a single window with lots of easy to use shortcuts (for example filtering between severity level). It’s easy to filter through files or even skip some errors entirely.

For example, here’s a screenshot where I could easily disable warnings found inside gtest.h which is a part of Google testing framework:

I won’t be able to fix those issues (as it’s third party code), so it’s best to make them silent.

Depending on your project size, you’ll probably need some time to adjust the output to your needs. After those adjustments, you’ll be able to focus on the major problems and limit the number of false positives or non-essential issues.

Here’s some more documentation if you want to start with your project.

• Getting acquainted with the PVS-Studio static code analyser on Windows
• How to run PVS-Studio on Linux and macOS

What’s more, you can also try PVS-Studio for free through Compiler Explorer! Have a look at this website how to start: Online Examples (C, C++).

Ok, but let’s see what the tool reported for my project.

Checking my project  

In total the analyser found 137 warnings and 8 criticals. We won’t cover them all, but for the purpose of this text, I grouped them and focused on the essential aspects.

Typos and copy-paste bugs  

The first one

friend bool operator== (const VECTOR3D& a, const VECTOR3D& b) { return (a.x == b.y && a.y == b.y && a.z == b.z); }

Do you see the error?

.

.

.

Maybe it’s quite easy when there’s only one function listed in the code sample, but it’s very easy to skip something when you have a bunch of similar functions:

bool operator== (const VECTOR3D& a, const VECTOR3D& b) { return (a.x == b.y && a.y == b.y && a.z == b.z); }
bool operator!= (const VECTOR3D& a, const VECTOR3D& b) { return (a.x != b.y || a.y != b.y || a.z != b.z); }
VECTOR3D operator- (const VECTOR3D& a)				   { return VECTOR3D(-a.x, -a.y, -a.z); }
VECTOR3D operator+ (const VECTOR3D& a, const VECTOR3D& b) { return VECTOR3D(a.x+b.x, a.y+b.y, a.z+b.z); }
VECTOR3D operator- (const VECTOR3D& a, const VECTOR3D& b) { return VECTOR3D(a.x-b.x, a.y-b.y, a.z-b.z); }
VECTOR3D operator* (const VECTOR3D& a, float v) 		  { return VECTOR3D(a.x*v, a.y*v, a.z*v); }
VECTOR3D operator* (float v, const VECTOR3D& a) 		  { return VECTOR3D(a.x*v, a.y*v, a.z*v); }

Copy-paste bugs or simple omissions can happen quite quickly… at least in my case :)

PVS -Studio reported the following message:

V1013 Suspicious subexpression a.x == b.y in a sequence of similar comparisons. tg_math.h 182

I guess it would be tough to spot this error, not easily at runtime.

Or another crazy and harmful bug:

for (i = 0; i < 4; i++)
	for (j = 0; j < 4; j++)
		buf.M[i][i] = M[i][i]*v;

For matrix multiplication… do you see the issue?

V756 The ‘j’ counter is not used inside a nested loop. Consider inspecting usage of ‘i’ counter. tg_math.cpp 157

Apparently, my code didn’t use that much of matrix transformations as I didn’t notice any issues at runtime, but it would be tricky to pinpoint the problem here.

The tool could detect even the following, however harmless issue (possibly as a result of copy paste):

inline float QuaternionNorm2(const QUATERNION_PTR q) { return ((q->w*q->w + q->x*q->x + q->y*q->y + q->z*q->z)); }

V592 The expression was enclosed by parentheses twice: ((expression)). One pair of parentheses is unnecessary, or misprint is present. tg_math.h 596

Such copy-paste bugs are very well described as the “Last Line Effect” - see The last line effect explained.

Let’s see some other issues:

Fixing a function  

Have a look

void DrawCylinder(float r, float h, int nx, int ny, 
                  bool spread, bool top, bool bottom) {
	// some general code...

	if (top == true) {
        // draw circle with triangle fan
	}

	if (top == true) {
        // draw circle with triangle fan
	}
}

This is a simple function that draws a cylinder with optional top and bottom sides.

And the errors?

V581 The conditional expressions of the ‘if’ statements situated alongside each other are identical. Check lines: 286, 305. gl_shapes.cpp 305 V751 Parameter ‘bottom’ is not used inside function body. gl_shapes.cpp 246

I haven’t seen this issue as a bug, because in the project, I always pass true for the top and the bottom parameters. But it’s clear that there could be a different case and my code would draw both sides wrongly.

Note: this bug could also be suggested by C4100 - MSVC warning enabled for Warning Level 4.

PVS-Studio makes it more evident that there’s something wrong with the similar code sections and that way it’s easier to have a look and recall what the real intention of the code was.

Omissions  

A quite common bug with enums:

switch (cmMode) {
		case cmYawPitchRoll: {
			// ..
			break;
		}
		case cmSpherical: {
			// ...   
			break;
		}
	}

V719 The switch statement does not cover all values of the ‘CameraMode’ enum: cmUVN. gl_camera.cpp 50

Such bugs can often arise when you extend the enum with new values, and you forget to update switch places where the enum is tested.

Missing Initialisation of Data Members  

Another critical bug that might cost you a lot of head-scratching:

V730 [CWE-457] Not all members of a class are initialized inside the constructor. Consider inspecting: m_fYaw, m_fPitch, m_fRoll, m_fHNear, m_fHFar. gl_camera.cpp 16

Fortunately, since C++11 we should use in-class member initialisation (see my separate blog post on that), but those bugs might be relatively often for legacy code.

Optimisation  

The analyser can also help to address performance issues. For example:

• Passing by reference:
  • V813 Decreased performance. The ‘filename’ argument should probably be rendered as a constant reference. clog.cpp 41
  • Often happens when you forget to add & when writing the type of the input argument.
• A better layout for structures:
  • V802 On 64-bit platform, structure size can be reduced from 72 to 64 bytes by rearranging the fields according to their sizes in decreasing order. ctimer.h 14
• List initialisation in constructors:
  • Test(const string& str) { m_str = str;} is less efficient than initialisaiton with m_str(str).

64 bit And Casting  

Issues with numbers and conversions might be tricky to address, but PVS-Studio can show you many things that might be important to fix. For example:

V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being cast: ’m_randomOrder.size()’. calgorithms.cpp 449

For this code:

if (m_i < static_cast<int>(m_randomOrder.size()))
// m_i is size_t, I changed it from int previously

Or the following report:

V108 Incorrect index type: numbers[not a memsize-type]. Use memsize type instead. av_data.cpp 41

For:

m_vCurrPos[i] += (numbers[i] - m_vCurrPos[i]) * s_AnimBlendFactor;

Floating point!  

Not to mention floating-point errors! Like this one:

V550 [CWE-682] An odd precise comparison: a.x == b.y. It’s probably better to use a comparison with defined precision: fabs(A - B) < Epsilon. tg_math.h 134

For the place when I compare floating-point values using == rather than fabs or some other functions that have some “epsilon”.

And even worse scenarios:

for (x = -4.0f; x < 4.0f; x+=1.0f) {
    for (z = -4.0f; z < 4.0f; z+=1.0f) {
    	// ...
    }
}

The above code generates:

V1034 [CWE-834] Do not use real type variables as loop counters. main.cpp 398

The code worked in my case, and this was used to draw some tiles on the floor… but it’s not the best approach and definitely not scalable.

Giving more check with MISRA  

While I wrote my project just for fun and without any “critical safety” in mind, it’s also noteworthy that PVS-Studio supports strict industry standards and guidelines that can strengthen your code.

To make it short, you can enable MISRA Coding standard checks and see how it works against your project. In my caste I got…

608 errors!

From what I see from the output it’s mostly about using unions (they are not safe in most of the cases). Some other bugs were related to literal suffix V2517. MISRA. Literal suffixes should not contain lowercase characters. And errors like:

• V2533 [MISRA C++ 5-2-4] C-style and functional notation casts should not be performed. tg_math.h 325

• V2564 [MISRA C++ 5-0-5] There should be no ‘integral to floating’ implicit cast. Consider inspecting the left operand ‘1’ of the operator ‘-’. gl_text.cpp 59

• Style guides

A lot of them were duplicates, so I need some time to sort them out.

Anyway if you like to read more about MISRA here’s a good starting point: What Is MISRA and how to Cook It

Summary  

Having a reliable static analysis tool helped me to identify a bunch of issues in my small project. I’m especially impressed with finding copy&paste kind of bugs which are easy to skip but can hurt a lot at runtime.

Here’s a wrap up of the strong points for the PVS-Studio:

• Super easy to install and run from Visual Studio.
• Nice and intuitive UI.
• Lots of filtering options, especially useful for large projects with potentially thousands of messages.
• Easy way to double click on the warning code and see a website with the information about a given rule.
• Great documentation, articles, community and PVS-Studio Release History.

Some things to improve:

• It’s tough to pick anything! It simply works and helps in your daily coding routine
• Maybe one thing, that you have to spend some time to tune the output to your project needs, some issues might not be essential and not relevant to your code.

The natural way to try the analyser on your code is to get the trial version. With the hashtag #bfilipek in the request form, the license key will be generated not for a week, but for a month.