Cppcheck vcmi

International board Development
#1

Good evening, I’ve run vcmi source code through cppcheck (analysis tool) and found some warnings. Attached patch which fixes warnings that aren’t severe. Deleted some unused functions, fixed & and && typo (I suppose this is the most major change :))

Everything compiles but I haven’t checked whether it works or not - don’t want to install it system-wide.
diff.txt (17.2 KB)

#2

Will commit, I do remember running cppcheck on our code some time ago but I haven’t found anything of interest back that time.

BTW - with C++11 std::list::size() has constant complexity so most of those micro-optimizations won’t change anything. This change will only affect readability.

#3

Yes, I read about C++11 and C++03 and complexity but it doesn’t mean vcmi is compiled only with compilers that support c++11.

Actually cppcheck produced a lot of warnings, though some of them seem to be false-positive (like “prefer ++i instead of i++”), speaking about others - I don’t know how to fix them.

#4

It is. We use C++11 functionality quite a lot - pretty much everything that is supported by gcc-4.6 and MSVC 2012.
How these compilers implement size() method right now is another question.

That’s pretty much false positive. I see no reason to “fix” it - if compiler can’t optimize code like that then I’d suggest to change compiler.

Micro-optimizations like this make sense only for readability. And personally I consider “i++” to be more easy to read than “++i”.

#5

Yeah, I know about ++i/i++, afaik now compilers handle these expressions the same way. So I just ignore these ‘warnings’.

#6

Done. Patch committed as revision 3602.

#7

in case you’re interested, here is cppcheck report in xml format. Much more easier to analyze it opening with cppcheck-gui. (rename back to .xml from .txt)
report.txt (140 KB)

#8

Hi KroArtem, can you tell how you’re using cppcheck? I’m trying to re-run it on updated code but at most I get 5-10 messages - much less than in yours. I’ve tried to enable all checks mentioned in help but still no changes.

#9

Hello Ivan, actually I build cppcheck from git master, then build cppcheck-gui (from the same repo) and run cpp-check-gui. Then its’ usage is quite obvious. :slight_smile: Doesn’t need to install system-wide.
When I didn’t know about gui for it, I used such .sh file:

#!/usr/bin/env sh

cppcheck -j `cat /proc/cpuinfo | grep processor | wc -l` --enable=all -f \
src/ 2> cppcheck-output
#10

Ah, got it - stupid typo on my side. I’m too lazy to install gui but console version now works just fine.

#11

Ok, moved to this thread as beegee is right.
Here is the link to coverity:
scan.coverity.com/projects/1005
I’ve tried to submit build but it failed due to some errors.

I’ve deleted those directory by mistake, will recompile and provide build-log.txt here soon.

#12

Here it is. As far as I understand, you need to fix these errors. Find errors by "WARNING: cov-emit ".
build-log.txt (2.12 MB)

#13

If I got correctly this is the only error:

"/usr/include/c++/4.8/array", line 162: error #2427: a constexpr member
          function is only permitted in a literal class type
        size() const noexcept { return _Nm; }
        ^
          detected during instantiation of class
                    "std::array<_Tp, _Nm> [with _Tp=BattleHex, _Nm=187UL]" at
                    line 142 of
                    "~/SVN/vcmi/client/../lib/CBattleCallback.h"

This errors is triggered for every case where we use std::array - for me it looks too much like compiler/standard library error. Our code seems to be OK here.

#14

Hm, not sure, maybe coverity-analysis tool is not compatible with gcc 4.8, I’ve got gcc version 4.8.1 (Ubuntu/Linaro 4.8.1-10ubuntu8)

#15

I was trying to build vcmi with coverity analysis tool to submit it to Coverity Online Scan. We use it in SuperTuxKart project and it already helped a lot.

However, it requires at least 85% of code to be compiled without errors. It’s the second time I didn’t manage to do so.

For example:

"/usr/include/c++/4.8/array", line 162: error #2427: a constexpr member
          function is only permitted in a literal class type
        size() const noexcept { return _Nm; }
        ^
          detected during instantiation of class
                    "std::array<_Tp, _Nm> [with _Tp=BattleHex, _Nm=187UL]" at
                    line 142 of "/home/kroartem/SVN/vcmi/lib/CBattleCallback.h"

Emit for file '/home/kroartem/SVN/vcmi/lib/CObjectHandler.cpp' complete.
1 error detected in the compilation of "/home/kroartem/SVN/vcmi/lib/CObjectHandler.cpp".
WARNING: cov-emit returned with code 2

In case everything is ok it says: Emit for file ‘/home/kroartem/SVN/vcmi/AI/FuzzyLite/LinguisticVariable.cpp’ complete.

Added build-log in case somebody is interested.

We’ve already fixed several memory leaks in SuperTuxKart, some copy-paste errors, initializing in a condition (for example, “if x=0” instead of if “x==0”), mismatching allocation/deallocation (free() instead of delete).

P.S. vcmi is already registered there (by me): scan.coverity.com/projects/1005
build-log.txt (2.79 MB)

#16

Maybe coverity doesn’t like some C++11 bits we’re using? For example this error makes no sense for me:

"/home/kroartem/SVN/vcmi/lib/CGameState.cpp", line 2334: warning #313: no
          suitable user-defined conversion from
          "lambda ](const EventCondition &)->bool" to
          "std::function<bool (const EventCondition &)>" exists
  		if ((event.trigger.test(evaluateEvent)))

Lamdas should be convertible to std::function with same signature by standard. Same goes to std::array::size() - that’s compile time constant so it can be turned into constexpr (not sure whether this function has to be consexpr or not)

Maybe move these posts to this thread?
[forum.vcmi.eu/t/cppcheck-vcmi/739/1)

#17

Coverity online scan uses 6.6.1 version. In case you’re sure all these errors are invalid, tell me and I’ll try to contact coverity’s support.

#18

Actually there is only one “error”. Quite a lot of these messages are warnings. And yes - notable number of those makes no sense for me.

The only error (and likely the only reason why you can’t submit vcmi code) is this one:

"/usr/include/c++/4.8/array", line 162: error #2427: a constexpr member 
           function is only permitted in a literal class type

Let’s look in standard (or in its final draft):

  1. According to standard std::array::size() should be contexpr so its presence is correct here. Perhaps std::array is not a literal class?

  2. std::array should be literal class if underlying class (BattleHex in this case) is literal type

  3. A type is a literal type if class has all of the following properties:
    …
    — it is an aggregate type (8.5.1) or has at least one constexpr constructor or constructor template
    that is not a copy or move constructor.

  4. BattleHex constructor is not marked as constexpr. But:
    … If that user-written default constructor would satisfy the requirements of a constexpr constructor (7.1.5), the implicitly-defined default constructor is constexpr.
    So BattleHex constructor should become constexpr.

(And why I can’t find that button that allows me to move posts to another topic?)

I don’t remember any threading issues from console commands (although I think that they are possible, with good timing). But merging may not be possible - currently on Win console waits inside getLine() until player enters something while Unix code enters that block only after input from player. So until somebody will merge Win part with Unix part merging is not possible.

Maybe bit more complicated but hopefully more general solution - one worker thread that executes events from all sources (console, network, GUI)

#19

So what?..

#20

That means that unless I missed something our code is correct and the problem is on Coverity side. Fact that VCMI compiles just fine by at least 3 compilers also speaks in our favor.