Skip to content
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

Integration of clang-tidy in CI #2745

Merged
merged 2 commits into from
Nov 6, 2023
Merged

Integration of clang-tidy in CI #2745

merged 2 commits into from
Nov 6, 2023

Conversation

daljit46
Copy link
Member

@daljit46 daljit46 commented Oct 30, 2023

One of the primary aims of #2689 was to facilitate the integration of our codebase with IDEs and other third party tools. This PR adds support for integrating clang-tidy, a linter tool for C++ source files, in our CI workflow. Static analysers are very useful to catch bugs and enforce stricter guidelines than your compiler's default warnings, and clang-tidy is arguably the most used and well-known of all of them.
Now, we cannot enforce any particular set of guidelines in one go, as that would be unfeasible (and probably not a good idea). For this reason, this PR includes the integration of the clang-tidy-review GitHub action, which creates a pull request review based on clang-tidy warnings.
These warnings then, hopefully, will be useful to contributors to improve the quality of their code. Note, however, that these checks won't be a point of failure (I think this is the most reasonable thing to do since our codebase isn't clang-tidy clean).

One important thing to discuss would be: what checks should be included in the warnings? Clang-tidy has an extensive list of available checks and allows defining a custom configuration file (see .clang-tidy file in this PR). I have created a starting configuration file, which I think is reasonable, but we should discuss this further (especially on checks that are intended to improve readability).

You can run clang-tidy locally on a file as follows:

cmake -B build
clang-tidy -p build /path/to/cppsourcefile

Also, many IDEs (e.g. Qt Creator) comes with built-in support for clang-tidy or provide an easy way to integrate it.

@github-actions
Copy link

clang-tidy review says "All clean, LGTM! 👍"

@daljit46
Copy link
Member Author

daljit46 commented Nov 2, 2023

@MRtrix3/mrtrix3-devs it'd be great if anyone could try out to run clang-tidy in your IDE and see the warnings it produces. There may some warnings that are either too noisy or superfluous.
So unless anyone has objections, I would like to merge this is as it is, since we can always improve the clang-tidy configuration in future PRs.

@daljit46 daljit46 requested a review from a team November 2, 2023 10:55
@daljit46 daljit46 merged commit 862142d into dev Nov 6, 2023
6 checks passed
@daljit46 daljit46 deleted the clang-tidy branch November 6, 2023 09:47
@Lestropie Lestropie mentioned this pull request Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant