Add clang-tidy to the code base #60
Draft
Conversation
This file contains hidden or 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
23 times, most recently
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}") |
Member
Author
|
Thank you! |
|
Merged
ericonr
changed the title
ericonr
force-pushed
the
static-analysis
branch
2 times, most recently
from
589502e to
e1df251
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, as was cppcoreguidelines, because it's harder to apply to a codebase that wasn't using it from the start. The specific tests from the groups in use that had to be removed are explained below: - bugprone-empty-catch: too eager, recommends longer code using find() member function; - concurrency-mt-unsafe: flags usage of exit(3) and strerror(3), which is basically noise. exit(3) has to be called by someone, and strerror(3) is thread-safe on the common case with locale set up and valid error values; - 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; - performance-avoid-endl: we want to be able to use endl; - 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; - readability-named-parameter: parameters are named in implementation files as a design choice. 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.
Reported-by: clang-tidy (bugprone-casting-through-void)
set_defaults for isla216p should be a member function, when it's implemented. Reference 'this' in the function body so clang-tidy doesn't try to turn it into a static function.
These were applied using the command below. clang-tidy --fix --fix-notes '--warnings-as-errors=-*' -p build/ \ $(git ls-files | grep '\.c\+$') Automatic fixes were reviewed and some changed manually, mainly to keep formatting consistency. Reported-by: clang-tidy
The check for GITHUB_HEAD_REF is used so the same workflow works for PRs and for branches. 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
|
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