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
Post a Comment