Skip to content

CI: Add CI checks with clang-tidy #454

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hallfox
Copy link
Collaborator

@hallfox hallfox commented Oct 9, 2024

This adds a new check to our workflows using clang-tidy. Right now we only enable analyzer checks, though we could look at expanding to more in the future. We are primarily interested in getting another pass over our code checking for C++03 syntax compatibility, since we've had a few instances of modern syntax constructs getting into the code base. clang-tidy will do a bit more than syntax checks, but I believe having will be a boon overall.

@hallfox hallfox added the A-Other Area: Other label Oct 9, 2024
@hallfox hallfox requested a review from a team as a code owner October 9, 2024 20:59
@hallfox hallfox force-pushed the syntax-checks branch 2 times, most recently from db3f89a to ac60326 Compare October 9, 2024 21:00
@hallfox hallfox changed the title Add CI checks with clang-tidy CI: Add CI checks with clang-tidy Oct 9, 2024
@hallfox hallfox force-pushed the syntax-checks branch 4 times, most recently from 8d0fcce to 1020655 Compare October 9, 2024 21:04
Signed-off-by: Taylor Foxhall <[email protected]>
Copy link
Collaborator

@pniedzielski pniedzielski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me; however I think the PR should also fix the issues clang-tidy finds, so we're not left with a failing CI. Maybe it makes sense to start out with a very small set of checks and have more PRs to incrementally enable more.

"inherits": "base"
},
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good change even beyond clang-tidy. Simplifies CI a lot.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming back to this and in reference to our (@hallfox, @emelialei88) offline discussion earlier today, it may be best to pull this out into its own PR (plus other useful presets).

I'll take a stab at this tomorrow and send it your way.

@pniedzielski
Copy link
Collaborator

@hallfox

Work I am doing in the same vein as #706 is to make the clang-format, black, etc checks we do in CI first-class CMake targets. It may make sense to add a clang-tidy CMake target first, fix the issues it finds, then update this PR to use that CMake target.

Thoughts?

@pniedzielski pniedzielski self-assigned this Apr 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Other Area: Other
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants