commit c719855806a9170608d2e374cc5c48b1ff661ece
author Ed Tanous <edtanous@google.com> Fri Jul 01 08:15:50 2022 -0700
committer Andrew Geissler <andrew@geissonator.com> Mon Jul 11 13:32:40 2022 +0000
tree 7dbf567e989e6e488f2f63c8a6d38c60d79d1d51
parent 6c55c580c9ab634e46ff6ab6732e06ad6a87e103

Improve cppcheck results and calling code

In the ideal case, cppcheck would have an option where it could be
enforcing in the future.  This commit works toward that goal, but
doesn't quite get there.

First, it updates cppcheck to the latest copy from github, instead of
pulling through apt sources.  This is done because the current cppcheck
has an issue with std::string_view, and erroneously believes that it
should be passed by reference.  The master branch has a fix for this.

Next, it modifies the call of cppcheck in a couple ways, first, to rely
on compile_commands.json from the build step, rather than attempting to
build a list of files itself.  This is less error prone, and allows
"whole binary" scans with cppcheck, which leads to fewer false
positives.  On the negative side, this means that non-meson/non-cmake
projects will no longer get scanned, which, considering that meson is
what we're converging on, and for any given project a meson port should
probably take priority over cppcheck improvements, this seems
reasonable.

Because of the previous change, the use of popen and communicate() can
be replaced with our more normal methods for calling commands, which
cleans up the code quite a bit.

Adds a cppcheck-temp folder as recommended by the cppcheck docs for
storing intermediate results.  This decreases the amount of time
cppcheck takes to run when scanning the same code multiple times.

After that, some argument changes are done to suppress checks that are
noisy, and the project doesn't currently have a way to make pass.

useStlAlgorithm this is just a suggestion, and not always correct, so
it's difficult to make it enforcing.  Also, as a project, we tend to not
optimize for std::algorithms as much as we could.  At some point in the
future we could turn this back on and make the project have a stronger
opinion on this style, but for the moment, this seems like something to
tackle later.
unusedStructMember this appears to either have bugs, or not understand
when a public struct member is used outside of the compile unit.  It
leads to significant noise for things that are actually used, but get
marked unused because they're not used in every compile unit.
postfixOperator use of prefix increment indead of postfix on iterators
is not generally something we enforce on the project, and doesn't seem
like it improves in either the readability or the code, as well as leads
to a lot of noise.  Disable for the moment.
unreadVariable Similar to unusedStructMember, this check seems to not be
able to see all possible ifdefs in a section of code, and leads to many
false positives.
knownConditionTrueFalse is supressed because apparently cppcheck isn't
able to read macros, so this flags significant portions of code behind
ifdefs.  More investigation is warranted here, but for the moment,
disabling the check gets us closer to the goal

A patch series getting bmcweb code to the point where it's 100% passing
is here, if you want to see the types of changes that cppcheck proposes.
For all the non-disabled checks, cppcheck seems accurate, with no false
positives that I could find, although some slightly pedantic
suggestions.

https://gerrit.openbmc.org/c/openbmc/bmcweb/+/55001

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I7730cc69ad1817cb7d75c04be3b7d1da6c1030d4