chore: Clang-tidy check headers #1440
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1440 +/- ##
==========================================
- Coverage 48.65% 48.63% -0.02%
==========================================
Files 381 381
Lines 20780 20771 -9
Branches 9518 9518
==========================================
- Hits 10110 10102 -8
+ Misses 4099 4098 -1
Partials 6571 6571
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Hm, this now seems to have a number of false negatives. |
|
I bumped up clang-tidy to v16 to see if that fixes the false negatives (i.e. warnings that are incorrect), but no luck. Not sure how to proceed with this then. I can circumvent these with comment markers telling clang-tidy to ignore them, but that seems a bit fragile. |
|
I added an extra clang tidy config file that the header script applies. That's not necessarily great, but right now I don't see a great alternative. Thought? @andiwand @benjaminhuth @timadye |
fair enough. we can always improve on that but I think it is good to have a baseline merged |
|
I have to admit I did not understood every detail of the clang-tidy integration. It looks quite complicated in total, but I would agree that its great to have a working version and maybe later improve on this... |
Getting clang-tidy to run on headers is not straightforward. That's why it gets somewhat complicated unfortunately. But from the changeset, you can see that it does flag a number of notes that we can benefit from fixing. In total we now have the integrated CMake clang-tidy run based off of the compilation database, and then this header one, where I just use |
|
can you add a little wrap-up in the description @paulgessinger ? as far as I understood clang-tidy is a pain in the butt with header file implementations and you are using a work around here. but the warning/errors are not consistent with cpp clang-tidy results? edit: looks like you did it already 😄 |
|
The warnings/errors should be consistent. I think that in case some of the includes are not resolved, clang-tidy sometimes produces wrong results. The implicit bool conversion warnings that popped up here looked to me like it was defaulting to the C behavior that unknown types are I propose we merge this (with all the fixes), see if we get bogus warnings frequently, and revert the check part if that's the case. |
|
Just one question maybe: |
|
Ok, so maybe this is not the best way to do it. For the checks that run through CMake I wanted to set the enabled checks directly in CMake. I added the config in Do you have any other ideas how to solve this better? |
|
Okay, that sounds reansonable. Let's merge then! |
|
should the cmake clang-tidy config and |
|
You mean to generate it in the CI before the clang-tidy job runs? |
|
either generate |
|
Okay, if we change this I would argue against generating the file in cmake. This has no real advantage I think. Running the cmake-triggered clang-tidy with the global |
|
This issue/PR has been automatically marked as stale because it has not had recent activity. The stale label will be removed if any interaction occurs. |
paulgessinger
force-pushed
the
chore/clang-tidy-headers
branch
from
d2bda2d
to
523fa31
Compare
📊 Physics performance monitoring for 523fa31Full report VertexingIVF seededIVF truth smearedIVF truth estimatedCKFseededtruth smearedtruth estimatedAmbiguity resolutionseededTruth trackingTruth tracking |
fix clang tidy for header files. also includes fixes for clang-tidy warnings from @paulgessinger replacement for #1440 Co-authored-by: Paul Gessinger <[email protected]>
fix clang tidy for header files. also includes fixes for clang-tidy warnings from @paulgessinger replacement for acts-project#1440 Co-authored-by: Paul Gessinger <[email protected]>
No description provided.