Add some static analysis to the code base. #60
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ericonr
commented
Jan 8, 2025
ericonr
force-pushed
the
static-analysis
branch
21 times, most recently
from
1042934 to
3ed0ad5
Compare
The test groups that were added were the ones considered relevant for this project. Test groups used to enforce code style for specific projects and platforms were omitted. The specific tests from the groups in use that had to be removed are explained below: - cppcoreguidelines-avoid-do-while: we use do-while for actual logic, there's no reason to remove it; - cppcoreguidelines-pro-bounds-pointer-arithmetic: pointer arithmetic is necessary in quite a few places. One of those is manipulating argv/argc, where using std::span doesn't add any safety (.at() member function is C++26), and std::vector requires copying; - cppcoreguidelines-pro-type-vararg: we use cstdio extensively; - modernize-use-trailing-return-type: though more modern, this function declaration style isn't widespread, and would conflict with the style used in other projects this one interacts with. It would also increase the barrier to entry for this project; - readability-braces-around-statements: braces are skipped for single statements, which should always be indented (this will be guaranteed by clang-format in the future); - readability-identifier-length: short names for iterator variables and throwaway values, as well as known shorthand (f for FILEs, v for verbose) should be used; - readability-implicit-bool-conversion: treating pointers as bools is a common idiom. We want clang-tidy to verify our own headers, which are under include/modules/ and utils/. We also want clang-tidy-diff, when used, to exit with an error.
Without this change, cppcheck would accuse that target_reg was uninitialized when used, even though a reference had been captured, and the lambda was only called after it was initialized. We can reorder the code so this false-positive isn't triggered, and without accidentally suppresing any warnings from other tools (as might have happened if we had initialized target_reg to nullptr).
ericonr
force-pushed
the
static-analysis
branch
from
3ed0ad5 to
0deceb0
Compare
Use a matrix with jobs in order to parallelize running different tools.
The check for GITHUB_HEAD_REF is used so the same workflow works for PRs
and for branches.
We ignore some files with cppcheck due to issues with Catch2. An example
of an error generated by cppcheck with Catch2:
util/tests/bits-test.cc:36:5: error: There is an unknown macro here
somewhere. Configuration is required. If _catch_sr is a macro then
please configure it. [unknownMacro]
CHECK_THROWS_AS(clear_and_insert(reg, 1000U, range_mask), std::runtime_error);
The issue causing git-config(1) to be necessary is tracked in [1].
Meson supports a clang-tidy target of its own, but it's not really
usable for our purposes [2].
[1] actions/checkout#1169
[2] mesonbuild/meson#2383
ericonr
force-pushed
the
static-analysis
branch
from
0deceb0 to
e4dfb45
Compare
|
Some checks that could be omitted (imo) in
|
|
With a regex-based python script, could find each occurrence and how many times it happens. Hope it's helpful. |
|
The script, if want to reproduce the results: import re
from collections import Counter
with open('static-analysis.txt', 'r') as file:
log_data = file.read()
pattern = re.compile(r'warning:.*?\[([a-zA-Z\-]+(?:,[a-zA-Z\-]+)*)\]')
matches = pattern.findall(log_data)
all_warnings = []
for match in matches:
all_warnings.extend(match.split(','))
counter = Counter([warning.strip() for warning in all_warnings])
for warning, count in counter.most_common():
print(f"{warning}: {count}") |
|
Thank you! |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Testing at least two options: clang-tidy+analyzer, and cppcheck
Could try and use https://github.com/SonarSource/sonarqube-scan-action , but requires creating an account and stuff, even if supposedly free for open source (does seem to be free: https://www.sonarsource.com/solutions/commitment-to-open-source/)
Would like to use GCC -fanalyzer, but so far it's not recommended for C++
https://github.com/cpplint/cpplint?tab=readme-ov-file is google-specific, but could try it at least locally to see where it complains and if it's compatible
There's also https://github.com/verateam/vera
Might be best to have a separate job for this, since I don't need to run it for every build. And separate jobs for each tool would also speed up things. Perhaps add flags to fail if errors are found?
Another nice addition would be running the testsuite under sanitizers