Issues reported by cppcheck

Based on widelands r6346 (with a few additional fixes not yet merged) and cppcheck 1.54.

Note that while generating your own report will list roughly ~40 more issues, but these already have suggested patches, so please don't waste your time doing duplicate work.

Also note that cppcheck 1.49-1 (at least in Ubuntu 11.10) will list all issues each time it finds it, while later versions only lists each issue only once. Without the duplicate lines, the report is a lot less noisy and easier to read. (The difference was ~260 vs ~12000 lines)

Seems cppcheck 1.55 was recently released, so I generated a new report with that based on the latest revision (r6411). Some hightlights from the report:

* Found a couple of memory leaks...
* ...and some variables nulled, but not freed
* usleep is obsolete
* It notes some places where replacement functions which are recommended for threadsafe applications
* Various variables not initialized in the constructor.
* Unused variables (some of these seem like they could be false positives to me, so please check those carefully if you intend to fix them)
* Variables passed as value when they could have been passed as value instead of reference
* Exceptions caught as values
* many more...

I merged your branch. Likely this does not make this bug fixcommitted, does it :).

Thanks for your work hjd!

Unfortunately, no, so hopefully someone else will take a look at the report. ;)

It should resolve four out of ~300 issues though.

Attached is the current report of cppcheck 1.55 at revision 6416.

Looks much better now :-)

A new version of cppcheck was recently released, so I tested lastet trunk with that to see whether it found any new issues:
* Some unused functions
* Reference to temporary returned
See previous comments...

Note that this report is longer than the one in the previous comment. This is mainly because a patch was reverted, as it introduced an unintended bug. It should probably be redone at some point in the future but we should be more careful not to break anything when doing so. See bug 1024696 for details.

Also, two files trigger an internal error in cppcheck. I filed a bug report on this (http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=686976)

As for the message at the bottom regarding files which are not included, I ran --check-config and it printed the following files:
[src/wui/overlay_manager.h:23]: (information) Include file: "boost/bind.hpp" not found.
[src/wui/overlay_manager.h:24]: (information) Include file: "boost/function.hpp" not found.
[src/minizip/unzip.h:53]: (information) Include file: "zlib.h" not found.
[src/minizip/zip.h:53]: (information) Include file: "zlib.h" not found.
[src/io/filesystem/filesystem.cc:25]: (information) Include file: "datafile.h" not found.
[src/minizip/unzip.cc:2543]: (information) Include file: "zlib.h" not found.
These looks to me like files belonging to various dependencies and probably out of scope for fixing issues in Widelands. I am not sure why they are not found, since I cannot remember seeing similar warnings/errors when compiling.

Adding a new report since a lot of "not initialized" issues were fixed in r6430. :)

New cppcheck release, new report.

Cppcheck 1.58 has been released. Hurray!

Now that we have GCC and Clang warning-free, issues like those in #6 should be easier to avoid as we can easier spot new warnings when they are introduced.

Implicit wink wink nudge nudge acknowledged :-)

Some of the reports smell like they may indicate memory leaks, which I am currently interested in, so I'm taking a look.

New cppcheck release recently and also lots of changes in the source code since the last time (including the merge of seafaring). This means it's time for a new report. :)

(Just a small heads-up; someone mentioned earlier that a lot of the methods listed as unused near the end are in fact used, but they are called from lua so they don't really show up. Therefore simply ripping all of them out might break quite a few things.)

We celebrate the recent release of cppcheck 1.61 by running it on r6698 to see what it has to say.

Looks like mostly the same things as last time, though a few comments:
- There's one issue about BOOST_FOREACH
- "src/network/nethost.cc:2237] -> [src/network/network_protocol.h:49: (style) Variable 'CLIENT_TIMESTAMP_INTERVAL' hides enumerator with same name" sounds potentially pretty bad, though I haven't looked closely at it.
- I recently saw someone create a branch which added threading to parts of WL. Cppcheck lists a lot of function calls which can be replaced by their threadsafe version, so if we are pushing towards threading we probably need to take a look at that.

Cppcheck 1.62, wl r6812.

Btw, since I can't remember seeing Mark for a long while now, I've taken the liberty of unassigning you. (No offense, merely so that others won't skip past this bug report because they believe someone else is working on it :) )

That's fine - things got a bit hectic for me!

Mark: No problem. You're welcome back when you find the time. :)

Cppcheck 1.64 is out. I ran it on r6857 and got the attached report. It is ~200 lines shorter than the previous one, so we are probably doing something right.

New report (nearly one hundred revisions worth of code changes has passed, and I fixed a couple of issues here the other day).

Please note that the unused variable in i18n.cc is a false positive. It is used, however only on certain platforms.

Cppcheck 1.65 was released yesterday, so here's an updated report! Regarding comment #16, this report is roughly the same size as the previous one. However, it contains lots of new warnings. :)

* It warns deallocation of an auto-variable results in undefined behaviour.
* Memory or resource leak in a few places. unzip.cc can be ignored, but someone should look into the others.
* Const variables are assigned an unnecessary copy of data which can be avoided by using a const reference instead.

Note that there are a couple of false positives where it claims variables are assigned a value which is never used. Though from what I can see, the variables are actually used for things like "if (not variable) { /*do stuff*/ }". Consequently , this also affects a couple of suggestions for reduced scope of variables. Please double-check these before fixing them.

Also, "Expression is always false because 'else if' condition matches previous condition" doesn't seem to look at the type used in dynamic_cast<> and marks them as identical.

We merged some pretty major changes recently, so I generated a new report.

Cppcheck 1.66 has been out for several hours, so it was about time to run it on the Widelands source code.

Good news first; they've fixed the false positives I mentioned in the previous release. :)

Some highlights of the issues found:
* structs without constructors
* classes without copy constructor even though they contain pointers to memory
* member variables not initialized in constructors.
* Boolean result is used in bitwise operation. The operator '!' and the comparison operators have higher precedence than bitwise operators.

Oh, and there's this check which I believe is new and I hope someone can look into:
src/ui_basic/progresswindow.cc:167: (error) Same iterator is used with different containers 'visualizations' and 'm_visualizations'.

I've taken some of the lowhanging fruit and created a branch, but there should be enough to go round in the report.

Latest check with recently released cppcheck 1.67.

New warnings:
- non-pure functions called inside asserts
I guess some of our method calls inside assert() might have side effects which means we could potentially get different behaviour between debug and release builds.
- conversion from const * char to string
Supposedly old parts of the code where we have only partially updated to use strings
- division by zero
though at a glance (read: random selection), these seem to point at string formatting, not divsion. So false positive?

Another thing it discovered was a method called selftest inside src/logic/cookie_priority_queue.h which is unused. It seems to be made for test purposes and I wonder whether it could be used as basis for a test suite for the class rather than simply deleting it.

> - non-pure functions called inside asserts

This is critical. That should never ever be the case.

[self-test]
Yes, having this turned into a test case would be much nicer. I wonder if it still passes it's self test :).

Somewhat delayed following cppcheck's release of 1.68, but here's an updated report.

Seems like both this and the previous one now notes whenever a configuration didn't need to be checked because it was identical to another one. This is a good thing because it saves time, but makes the reports rather noisy. I haven't found a way to silence this yet. :(

Still cppcheck 1.68, but rechecked against latest trunk. As I mentioned above, the reports get rather noisy with all the info about all the configurations which are identical to something which has been checked already. Therefore I've prepared a small patch which filters out these entries. This has not been merged yet, but attached is an example report.

New cppcheck release, new report.

Not sure what they've changed under the hood, but the past few releases has noticably increased runtime. With 1.69 it takes less than three minutes to scan the Widelands sources, where it used to take hours. And this is on a VM, on real hardware it probably runs faster still.

I had a look at some of the

     The function 'function_name' is never used

errors. They were all false positives so far, so going through these will take us longer than it's worth I think.

>They were all false positives so far, so going through these will take us longer than it's worth I think.

Yes, these are mostly (if not all) false positives. I don't remember when it was discussed, but someone mentioned that they were either called from lua or used in the tests. I don't really know how this usage could be included. :(

Actually the ones that I checked out were all called within C++. Some of them were non-class functions, which might also be a problem for cppcheck, e.g. i18n::set_locale.

>Actually the ones that I checked out were all called within C++.

Oh, I either misunderstood originally or these might be new. Possibly both :p

>Some of them were non-class functions, which might also be a problem for cppcheck

Sounds like it might be a bug in cppcheck then. If someone made a minimal example with a non-class function which reported a false positive, I suppose we might report it to the cppcheck project.

Cppcheck version 1.70 was released quite some time ago, but I see that I haven't run the usual release-celebrating scan.

Related to a discussion about bug 1509772, I thought an up-to-date report might be useful.

I'll clean up the asserts.

I had some time and ran cppcheck (version 1.74) against current trunk.

There are several errors about memory leaks and many performance hints.

New report with the latest stable release of cppcheck.

Still some memory leaks.

Some warnings for various Lua* classes (The class 'LuaMultiClass' defines member variable with name 'className' also defined in its parent class 'LuaClass'). These methods seems to originate from LUNA_CLASS_HEADER, though it looks like if there's an inheritance hierarchy they aren't marked as override and is seen as duplicates. Not sure if there's an easy fix here.

Lots of methods in WorkerProgram, for instance 'WorkerProgram::parse_mine', are marked as unused private functions, but I don't know whether these are actually unused or invoked via some lua magic.

The WorkerProgram::parse_... functions are used - they are registered in

    const WorkerProgram::ParseMap WorkerProgram::parsemap_[]

and then called in

    void WorkerProgram::parse(const LuaTable& table).

Regarding the memory leaks - only the following are potentially true positives:

src/logic/map_objects/bob.cc:544: (error) Memory leak: state.path
src/logic/map_objects/bob.cc:564: (error) Memory leak: state.path

Thew UI classes are memory managed by UI::Panel, so they can't leak.

New release, new report. Some highlights:
- possible operator precedence issues
- unused private functions (and some struct members)
- various methods also defined in parent class (not sure how to mark these as overriding or what the c++ terminology is)
- and a couple of more unused variables/values where I wasn't sure what the intetion was, so I haven't removed them.

New report with cppcheck 1.79. Haven't looked too much into the details, but seems to be some new issues.

GunChleoc has been chopping away to reduce the amount of warnings. Just run a new report with r8400 (nice round number), and looks like the report is 100kb shorter now. :)

Haven't looked much into the details, but should be easier to see the more severe issues now.

As a bonus, I've also generated a minimal report. The full report above took about 45 minutes to generate, while this one runs through the whole source code in less than one minute.

The logs might still be too noisey to compare them at this point, but if we could run the fast one without missing issues, I think that would make it easier to run it often. It also allows us to ignore third_party issues to keep them from cluttering up the log. (This can also be done for the full report, but since that includes the source code as well, they still show up :/ )

For those who want to try at home, this is the command to do a fast check:
`cppcheck --quiet --verbose --enable=all -i src/third_party/ src 2>&1 | grep -v "was not checked because its code equals another one" > cppcheck-minimal.txt`

(I am not sure why the "Return value of function log() is not used." only show up in the minimal report, furthermore I'm curious as to why log would have a return value...)

There is a C++ default function that is also called "log" and which returns a double

http://www.cplusplus.com/reference/cmath/log/

Our log function returns void. We can probably shut these up by explicitly including "base/log.h".

Well, that didn't work, so I have no idea how to get rid of the "log" false positives at this time, short of giving it a namespace, which would be annoying.

>Well, that didn't work, so I have no idea how to get rid of the "log" false positives at this time, short of giving it a namespace, which would be annoying.

Then it's probably easier to just filter then out when running the minial report.

In the meantime, there's still the "large" report to chip at, so I've attached an up to date one now that cppcheck 1.80 has been released.

When I'm finished with my mopup branch, the attached issues are the remaining "interesting" issues that should still be looked at.

Cppcheck 1.81 is out, attaching an up-to-date report.

New report based on cppcheck 1.82. Seems to be a couple of unused methods here and there, and methods called in assertions...

New cppcheck release (1.83), new report. :)

Migrated to https://github.com/widelands/widelands/issues/1406