-
Notifications
You must be signed in to change notification settings - Fork 51
Configure ci clang tidy report #338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Configure ci clang tidy report #338
Conversation
d256dec to
18cf424
Compare
|
Hello @rolandreichweinbmw @christian-schilling @hbragge-acn @johannes-esr let's maybe move the discussion here. In this PR I followed the suggestion of doing a full report of clang-tidy findings, instead of just checking a diff. I tried to fix some of the warnings, but I didn't do it for all of them. There is a commit that addresses only one of the checkers. I also focused on the posix build, and as such you will find in the s32k1xx build some warnings that supposedly were cleaned up in some of the commits. There is also some clang-diagnostic-error which I'm not yet sure how to solve. These happen because clang-tidy looks to each compilation unit that is written in the compile_commands.json, and apparently some of these have invalid targets for unit test builds, and because of this CMake ignores those targets, and as such the compile commands becomes incomplete. Let me know what you think. Also, the checkers that I introduced came from a previous project, so I'm not really enforcing them. We may remove the ones you don't want to have. |
rolandreichweinbmw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only put a few remarks.
Also: Are the included "Fix" commits directly related to clang-tidy findings? E.g. directly enabling a clean acceptable baseline? Otherwise, "unrelated" changes could be separated out to a cleanup PR.
Theoretically yes, each commit is respective to one clang-tidy finding. So, in principle, if I remove one commit the PR should remain clean, or at least not needed many rebase changes |
18cf424 to
d829127
Compare
Signed-off-by: Filipe Cuim <[email protected]>
Signed-off-by: Filipe Cuim <[email protected]>
Signed-off-by: Filipe Cuim <[email protected]>
Signed-off-by: Filipe Cuim <[email protected]>
Signed-off-by: Filipe Cuim <[email protected]>
Signed-off-by: Filipe Cuim <[email protected]>
Signed-off-by: Filipe Cuim <[email protected]>
Signed-off-by: Filipe Cuim <[email protected]>
Signed-off-by: Filipe Cuim <[email protected]>
Signed-off-by: Filipe Cuim <[email protected]>
Signed-off-by: Filipe Cuim <[email protected]>
d829127 to
7d9d4f7
Compare
Signed-off-by: Filipe Cuim <[email protected]>
Signed-off-by: Filipe Cuim <[email protected]>
Signed-off-by: Filipe Cuim <[email protected]>
Signed-off-by: Filipe Cuim <[email protected]>
7d9d4f7 to
74472ef
Compare
Description
The goal of this PR is to configure a .clang-tidy that respects openbsw coding conventions, and a ci job that runs clang-tidy checks on all compilation units that are generated in compile_commands.json.
In this first step we start with fewer clang-tidy checks enabled, because currently with all checks enabled the repository has around 3924 findings (excluding all files in test, 3rdparty and mock) and around 23386 findings (when excluding only 3rdparty folder).
Each clang-tidy CI workflow will print a report of all findings for each checker in stdout. After that it will also upload an artifact with a yaml file generated by clang-tidy, which will contain the location (both file name and file offset) of all findings.
There may also exist some checkers that we do not want to activate, and as such it's better to do small PRs after this one, where only few checkers are considered, in order to limit the amount of changes in the repository.