Skip to content

Add clang_tidy_test #85

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

Merged
merged 7 commits into from
Feb 20, 2025
Merged

Add clang_tidy_test #85

merged 7 commits into from
Feb 20, 2025

Conversation

keith
Copy link
Contributor

@keith keith commented Feb 14, 2025

This is a new test rule that executes clang-tidy as a test via shell
script instead of an aspect.

The biggest implementation issue with this is that the paths you get
from CcInfo do not match the directory structure when running clang-tidy
in a test. This requires a bit of fixing up.

This attempts to share as much logic as possible with the aspect. I
think a bit more could be shared in the future.

Based on #65

Closes: #65
Fixes: #15

@keith
Copy link
Contributor Author

keith commented Feb 14, 2025

One interesting difference with the test is it runs over multiple files at once, potentially only invoking clang-tidy once per library, but it will invoke it twice if there is a C file and C++ file

@keith
Copy link
Contributor Author

keith commented Feb 14, 2025

One interesting difference with the test is it runs over multiple files at once, potentially only invoking clang-tidy once per library, but it will invoke it twice if there is a C file and C++ file

im actually not sure if this is good or bad since clang-tidy seems to not run in parallel and is quite slow. but users can always make 1 test target per file if they'd prefer.

This is a new test rule that executes clang-tidy as a test via shell
script instead of an aspect.

The biggest implementation issue with this is that the paths you get
from CcInfo do not match the directory structure when running clang-tidy
in a test. This requires a bit of fixing up.

This attempts to share as much logic as possible with the aspect. I
think a bit more could be shared in the future.

Based on erenon#65

Closes: erenon#65
Fixes: erenon#15
@keith keith force-pushed the ks/add-clang_tidy_test branch from 99dd2c6 to 5e07a7b Compare February 14, 2025 18:55
@keith
Copy link
Contributor Author

keith commented Feb 14, 2025

added a data parameter so it's easy to add files without them being linted, which users can use in a macro to create 1 test target per file while still providing all the necessary files to allow clang-tidy to work

Copy link
Owner

@erenon erenon left a comment

Choose a reason for hiding this comment

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

Thanks for the high quality contribution!

@keith keith requested a review from erenon February 18, 2025 19:04
@keith
Copy link
Contributor Author

keith commented Feb 18, 2025

requires ci fix: #86

@erenon erenon merged commit a965fb4 into erenon:master Feb 20, 2025
1 check passed
@erenon
Copy link
Owner

erenon commented Feb 20, 2025

Thanks, merged!

@keith keith deleted the ks/add-clang_tidy_test branch February 20, 2025 17:32
@keith
Copy link
Contributor Author

keith commented Feb 20, 2025

thanks! sorry i should have squashed here, I assumed that would happen at merge time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is it possible to invoke this as a bazel test?
2 participants