static analysis of swarm's C++ code (cppcheck)

[frederic-mahe]

This is a minor issue. Maybe the two warnings are worth correcting.

# where swarm is swarm's repository
cppcheck --enable=all swarm/ 1> /dev/null

returns two warnings, and several variables whose scope could be reduced:

[swarm/src/derep.cc:346]: (warning) %u in format string (no. 3) requires 'unsigned int' but the argument type is 'signed int'.
[swarm/src/derep.cc:346]: (warning) %u in format string (no. 4) requires 'unsigned int' but the argument type is 'signed int'.

[swarm/src/bitmap.h:32]: (style) Class 'Bitmap' has a constructor with 1 argument that is not explicit.

[swarm/src/algo.cc:65]: (style) The scope of the variable 'swarmsize' can be reduced.
[swarm/src/db.cc:266]: (style) The scope of the variable 'm' can be reduced.
[swarm/src/nw.cc:128]: (style) The scope of the variable 'hep' can be reduced.
[swarm/src/swarm.cc:104]: (style) The scope of the variable 'b' can be reduced.
[swarm/src/swarm.cc:104]: (style) The scope of the variable 'c' can be reduced.
[swarm/src/swarm.cc:104]: (style) The scope of the variable 'd' can be reduced.

Regarding the scope reduction, is this what should be done for m?

     /* before */
      while (line[0] && (line[0] != '>'))
        {
          char m;
          char c;
          char * p = line;
          while((c = *p++))
            if ((m = map_nt[(int)c]) >= 0)
      ...

     /* after */
      while (line[0] && (line[0] != '>'))
        {
          char c;
          char * p = line;
          while((c = *p++))
            if ((char m = map_nt[(int)c]) >= 0)
      ...

edit: no, it does not seem possible to type the variable inside a conditional fork.

[frederic-mahe]

for swarmsize, it is not clear if it stores the size of one cluster, or the cumulated size of all clusters. If it concerns only a single cluster, then it should be defined inside this while loop:

  while (seeded < amplicons)
    {
      /* process each initial seed */
      ...
    }

[frederic-mahe]

The only remaining warnings are:

[swarm/src/bitmap.h:32]: (style) Class 'Bitmap' has a constructor with 1 argument that is not explicit.
[swarm/src/swarm.cc:104]: (style) The scope of the variable 'b' can be reduced.
[swarm/src/swarm.cc:104]: (style) The scope of the variable 'c' can be reduced.
[swarm/src/swarm.cc:104]: (style) The scope of the variable 'd' can be reduced.

[torognes]

I have checked the modifications and made some more to avoid all warnings from cppcheck, except in the cityhash code. All changes committed.

[frederic-mahe]

There are still a few warnings:

cppcheck --enable=all swarm/ 1> /dev/null
[swarm/src/swarm.cc:173]: (warning) %lu in format string (no. 1) requires 'unsigned long' but the argument type is 'long'.

[torognes]

Fixed.

[frederic-mahe]

Running again cppcheck on the latest master version of swarm v2:

cppcheck --enable=all ./swarm/ 1> /dev/null

cppcheck yields three warnings:

[swarm/src/algod1.cc:1410]: (warning) %d in format string (no. 2) requires 'int' but the argument type is 'unsigned int'.
[swarm/src/algod1.cc:1436]: (warning) %d in format string (no. 2) requires 'int' but the argument type is 'unsigned int'.
[swarm/src/algod1.cc:1502]: (warning) %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'.

The rest are style issues:

[swarm/src/cityhash/city.cc:171]: (style) The function 'CityHash32' is never used.
[swarm/src/cityhash/city.cc:386]: (style) The function 'CityHash64WithSeed' is never used.
[swarm/src/cityhash/city.cc:618]: (style) The function 'CityHashCrc128' is never used.
[swarm/src/cityhash/city.cc:605]: (style) The function 'CityHashCrc128WithSeed' is never used.
[swarm/src/db.cc:630]: (style) The function 'db_getheader' is never used.
[swarm/src/db.cc:635]: (style) The function 'db_getheaderlen' is never used.
[swarm/src/db.cc:597]: (style) The function 'db_getlongestheader' is never used.
[swarm/src/db.cc:607]: (style) The function 'db_getseqinfo' is never used.
[swarm/src/db.cc:645]: (style) The function 'db_putseq' is never used.
[swarm/src/search16.cc:31]: (style) The function 'dprofile_dump16' is never used.
[swarm/src/search8.cc:31]: (style) The function 'dprofile_dump8' is never used.
[swarm/src/search8.cc:54]: (style) The function 'dseq_dump8' is never used.
[swarm/src/algod1.cc:689]: (style) The function 'expected_variant_count' is never used.
[swarm/src/util.cc:140]: (style) The function 'hash_djb2' is never used.
[swarm/src/util.cc:155]: (style) The function 'hash_djb2a' is never used.
[swarm/src/util.cc:124]: (style) The function 'hash_fnv_1a_32' is never used.
[swarm/src/util.cc:108]: (style) The function 'hash_fnv_1a_64' is never used.
[swarm/src/scan.cc:211]: (style) The function 'master_dump' is never used.
[swarm/src/qgram.cc:43]: (style) The function 'printqgrams' is never used.
[swarm/src/matrix.cc:37]: (style) The function 'score_matrix_dump' is never used.
[swarm/src/db.cc:82]: (style) The function 'showseq' is never used.

These style issues are not really a problem, but maybe these unused functions could be commented or removed?

Running cppcheck on swarm 3.0 yields:

[swarm/src/algo.cc:61]: (style) C-style pointer casting
[swarm/src/algo.cc:62]: (style) C-style pointer casting
[swarm/src/algo.cc:562]: (style) The scope of the variable 'swarmcount' can be reduced.
[swarm/src/algod1.cc:1094]: (warning) %d in format string (no. 2) requires 'int' but the argument type is 'unsigned int'.
[swarm/src/algod1.cc:1120]: (warning) %d in format string (no. 2) requires 'int' but the argument type is 'unsigned int'.
[swarm/src/algod1.cc:547]: (style) C-style pointer casting
[swarm/src/city.cc:398]: (performance) Function parameter 'seed' should be passed by reference.
[swarm/src/city.cc:428]: (performance) Function parameter 'seed' should be passed by reference.
[swarm/src/city.cc:606]: (performance) Function parameter 'seed' should be passed by reference.

[torognes]

I've run cppcheck on Swarm 3.0 and removed all issues except those relating to CityHash. I prefer to mess as little as possible with that code.

[torognes]

Will fix issues in swarm 2 as well.

[torognes]

Swarm 2 updated too.

[frederic-mahe]

Now cppcheck only reports style issues, no more warnings. Here is the report for swarm 2, excluding cityhash:

[swarm/src/algo.cc:163] -> [swarm/src/algo.cc:294]: (style) Local variable listlen shadows outer variable
[swarm/src/algod1.cc:1039] -> [swarm/src/algod1.cc:1397]: (style) Local variable swarmid shadows outer variable
[swarm/src/algod1.cc:1039] -> [swarm/src/algod1.cc:1465]: (style) Local variable swarmid shadows outer variable
[swarm/src/db.cc:214] -> [swarm/src/db.cc:396]: (style) Local variable lineno shadows outer variable

Same thing for swarm 3.0:

[swarm/src/algo.cc:182] -> [swarm/src/algo.cc:321]: (style) Local variable listlen shadows outer variable
[swarm/src/algo.cc:567] -> [swarm/src/algo.cc:595]: (style) Local variable mass shadows outer variable
[swarm/src/algo.cc:569] -> [swarm/src/algo.cc:596]: (style) Local variable seed shadows outer variable
[swarm/src/algod1.cc:730] -> [swarm/src/algod1.cc:1075]: (style) Local variable swarmid shadows outer variable
[swarm/src/algod1.cc:730] -> [swarm/src/algod1.cc:1144]: (style) Local variable swarmid shadows outer variable
[swarm/src/db.cc:379] -> [swarm/src/db.cc:583]: (style) Local variable lineno shadows outer variable
[swarm/src/hashtable.h:24] -> [swarm/src/derep.cc:84]: (style) Local variable hash_mask shadows outer variable
[swarm/src/variants.cc:221] -> [swarm/src/variants.cc:243]: (style) Local variable base shadows outer variable

Nice work!

[torognes]

Fixed the shadowed variables in Swarm2 and Swarm3 as well.

[frederic-mahe]

Awesome! To conclude that journey, here is the cppcheck command one can use to ignore cityhash code for swarm 2:

cppcheck --enable=all -i "./swarm/src/cityhash/" ./swarm/ 1> /dev/null

and for swarm 3:

cppcheck --enable=all -i "./swarm/src/city.cc" ./swarm/ 1> /dev/null