Skip to main content

Use cppcheck to find bugs and improve code quality (not only for Kile)

Do you know isocpp.org's blog? As an open-minded C++ programmer, I am a fond reader and have been inspired multiple times. I always enjoyed the blog posts from Andrey Karpov. He has deep knowledge with static code analysis and is a co-founder of PVS-Studio, a commercial static code analyzer for C++, C#, C, and Java. To advertise new releases of their product, Andrey and his co-workers scan popular open source projects with their tool. They explain the numerous results and showcase by these real-world examples how beneficial static code analysis is even for mature and healthy code bases. I found these posts both entertaining and instructive. If you are not aware of them, you might find them an interesting read: Clang 11, LLVM 15, Qt 6, GCC 13. I find this topic intriguing; nevertheless, for a long time I did not manage to dive deeper into this topic.

I am a satisfied user of Kile, KDE's user-friendly TeX/LaTeX editor. In the span of almost 20 years (Is Kile really that old? When have I gotten this old?), I have typeset dozens of documents with hundreds of pages using Kile. To check Kile's status, I visited its project page. One thing is, that it has not seen a release for a long time. The other thing that caught my eye is the result of cppcheck, visible in every open merge request. Cppcheck is an open-source static code analyzer for C and C++. It tries to find undefined behavior, dangerous code patterns, and bad coding style. GitLab showed a staggering 300+ cppcheck findings for Kile's open merge requests.

Gitab shows for every Kile merge request cppcheck's code quality scan results
 

This does not mean Kile having 300 bugs! Most findings are harmless, but some real issues might hide between all these findings. Since June I reduced the number of cppcheck findings by almost 200. Thereby, we improved some bad code style and even found real bugs.

Unused variables or unused assigned values

Unused variables are variables that are in the code but are not used. Closely related are cases of assigned values, that are never used after the assignment. This should not happen, often it is a result of code changes with no longer variables or assignments left. I removed 17 such cases, and it makes the code clearer to understand.

Case closed? Not so fast. In one instance, it revealed a tiny bug.

01 QString icon;
02 if (ext == "application-x-bzdvi" ) {
03     icon = ext;
04 }
05 else if( ext == "htm" || ext == "html" ) {
06     icon = "text-html";
07 }
08 else if(ext == "pdf" ) {
09     icon = "application-pdf";
10 }
11 else if( ext == "txt") {
12     ext = "text-plain";
13 }
[..]
22 else {
23     icon = "text-plain";
24 }
25 return icon;

Cppcheck complained that in line 12 the variable ext gets assigned a value that is never used. Well, yes, because the value should be assigned to icon like in the other if branches. After fixing, text files gained an icon in Kile's documentation browser.

Document browser has now icons for text files, too. The light gray color is a new feature independent of the issue at hand.


Another bug went unnoticed by me, but Kile's maintainer Michel spotted it.

01 //both exist, take most recent
02 if(read1 && read2) {
03     read1 = file1.lastModified() > file2.lastModified();
04     read2 = !read1;
05 }
06
07 if(read1) {
08     dir = S();
09     trg = "index.html";
10
11    translate(dir);
12    setRelativeBaseDir(dir);
13    translate(trg);
14     setTarget(trg);
15 }

Cppchechk warned that in line 4 the value assigned to read2 is never used. I removed the line, but the actual fix was adding an else(read2) branch. This case was forgotten and spotted with a careful review of cppcheck's findings.

C-style pointer castings

C-style castings are tricky, as they are sometimes a promise from the programmer that the types are compatible and the compiler has no way of checking. With C++ there are five (!) different type of casts, many are not dangerous as the compiler can check the compatibility of what the programming is asking for. For more details, see a blog post from Anteru explaining more details.

Six cases of C-style pointer castings were not necessary and could be dropped. Twelve more could be replaced by C++'s static_casts, ensuring all safety checks from C++ are applied.

Other improvements

Many other improvements made the code more beautiful, like having variable scopes reduced as much as possible. This leads to variables that are only known inside a loop, and that variable declaration and its use stay closer together.

In one case cppcheck found a potential memory leak as a variable was created with new whenever the surrounding method was called, but the object was never deleted.

Two for loops with iterators could be slightly simplified by range-based for loops. The latter prevents off-by-one errors and expresses the intend more direct.

That is all?

I fixed only some of the most obvious findings. I spared the complicated ones, as my understand of Kile's code base is still limited. Further, I did not want to introduce new bugs to not endanger the hopefully upcoming release.

In the remaining cppcheck findings are some more potential bugs, their description looks promising.

 

Have a look at cppcheck findings yourself!

I can recommend to examine cppcheck findings for your project's, too. At first, it might look like a cumbersome task. I am sure it is worth the effort.

Fixing cppcheck findings is also a good junior job to get in touch with the development of your pet part of KDE. Start with some, not too many at once, trivial fixes and follow the feedback from the maintainer. My experience with Kile was pleasant, thanks Michel!

Comments

Popular posts from this blog

New programming language needed for KDE?

Disclaimer: I am not one of KDE's masterminds or spokespersons. I am a mere bystander with few unimportant commits. I follow KDE's ecosystem and other developments in the free software world. In the following, I share some thoughts and my personal opinion. Talks about new programming languages After 30 years of C code, the Linux kernel opens itself to a second high-level language: Rust. Since fall of 2022 the kernel mainly gained infrastructure work. Some experiments show promising results like a Rust-based network driver or a scheduler . Recently, Git developers started to discuss how to allow Rust code in our beloved version control system. Far from having reached a consensus, its media coverage and heated discussions in forums show how interested the public is in this topic. Other projects try to replace established software by rewritten from scratch Rust ones: uutils coreutils , sudo-rs , librsvg , Rustls . Heck, Rewrite it it Rust (RiiR) has become a meme . We already h...

Kile 2.9.95 / 3.0 beta 4 released

We have a release of Kile 2.9.95, also known as 3.0 beta 4! Earlier today, Michel Ludwig tagged the current Git master. This is the first beta release since October 2019. Beside the port to KDE Frameworks 6 and Qt 6, it provides a couple of new features and bug fixes. New features Port to KDE Frameworks 6 & Qt 6 (Port by Carl Schwan) Enable high-dpi support Provide option to hide menu bar (Patch by Daniel Fichtner, #372295 ) Configurable global default setting for the LivePreview engines (Patch by Florian Zumkeller-Quast, #450332 ) Remove offline copy of "LaTeX2e: An unofficial reference manual", use online version instead (Patch by myself, Christoph Grüninger, Issue #7 ) Fixed bugs Kile crashes on selecting "Browse" or "Zoom" for document preview (Patch by Carl Schwan, #465547 , #476207, #467435, #452618, #429452) Kile crashes when generating new document (Patch by Carl Schwan, #436837 ) Ensure \end{env} is inserted in the right place even when the...

Release of KDE Stopmotion 0.8.7

Today marks the release of  KDE Stopmotion 0.8.7 ! About Stopmotion Stopmotion is a Free Open Source application to create stop-motion animations. It helps you capture and edit the frames of your animation and export them as a single file. Direct capture from webcams, MiniDV cameras, and DSLR cameras. It offers onion-skinning, import images from disk, and time lapse photography. Stopmotion supports multiple scenes, frame editing, basic sound track, animation playback at different frame rates, and GIMP integration for image. Movies can be exported to a file and to Cinelerra frame lists. Technically, it is a C++ / Qt application with optional dependencies to camera capture libraries. Changes in release 0.8.7 This release comes with no new features, but improvements to the project itself. Changes The project is now officially called to KDE Stopmotion. The former name Linux Stopmotion is no longer used. Support for qmake has been removed. Use CMake instead. Features Port serial...