-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issues reported by cppcheck #1406
Comments
(by hjd) 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) |
(by hjd)
|
(by sirver) Thanks for your work hjd! |
(by hjd) It should resolve four out of ~300 issues though. |
(by qcumber-some) Looks much better now :-) |
(by hjd)
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: |
(by hjd) |
(by hjd) |
(by hjd) 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. |
(by mxsscott) Some of the reports smell like they may indicate memory leaks, which I am currently interested in, so I'm taking a look. |
(by hjd) (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.) |
(by hjd) Looks like mostly the same things as last time, though a few comments:
|
(by hjd) 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 :) ) |
(by mxsscott) |
(by hjd) |
(by hjd) |
(by hjd) Please note that the unused variable in i18n.cc is a false positive. It is used, however only on certain platforms. |
(by hjd)
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. |
(by hjd) |
(by hjd) Good news first; they've fixed the false positives I mentioned in the previous release. :) Some highlights of the issues found:
Oh, and there's this check which I believe is new and I hope someone can look into: I've taken some of the lowhanging fruit and created a branch, but there should be enough to go round in the report. |
(by hjd) New warnings:
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. |
(by sirver)
This is critical. That should never ever be the case. [self-test] |
(by hjd) 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. :( |
(by hjd) |
(by hjd) 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. |
(by gunchleoc)
errors. They were all false positives so far, so going through these will take us longer than it's worth I think. |
(by hjd)
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. :( |
(by gunchleoc) |
(by hjd)
Oh, I either misunderstood originally or these might be new. Possibly both :p
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. |
(by hjd) Related to a discussion about bug 1509772, I thought an up-to-date report might be useful. |
(by gunchleoc) |
(by franku) There are several errors about memory leaks and many performance hints. |
(by hjd) 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. |
(by gunchleoc)
and then called in
|
(by gunchleoc) src/logic/map_objects/bob.cc:544: (error) Memory leak: state.path Thew UI classes are memory managed by UI::Panel, so they can't leak. |
(by hjd)
|
(by hjd) |
(by hjd) Haven't looked much into the details, but should be easier to see the more severe issues now. |
(by hjd) 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: (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...) |
(by gunchleoc) http://www.cplusplus.com/reference/cmath/log/ Our log function returns void. We can probably shut these up by explicitly including "base/log.h". |
(by gunchleoc) |
(by hjd)
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. |
(by gunchleoc) |
(by hjd) |
(by hjd) |
(by hjd) |
Conclusion in https://www.widelands.org/forum/topic/5049/ was to not add Should we close this or check current results again and reconsider? |
Attached you should find an up to date report of the issues found by cppcheck. This is generated by running the script utils/create_cppcheck_report. Be aware that this takes a while to run.
It currently lists various issues, the majority seem to be parameters which should be passed as reference, values not initialized by the constructor.
PS. For other warning reports, see bug 1258667 (Clang), bug 1278174 (flawfinder) and bug 1202101 (Visual Studio).
Imported from Launchpad using lp2gh.
The text was updated successfully, but these errors were encountered: