Aug 16 '22 13:08 paulgessinger

Codecov Report

Merging #1440 (523fa31) into main (5c07814) will decrease coverage by 0.01%. The diff coverage is 54.54%.

@@            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

Impacted Files	Coverage Δ
...ude/Acts/MagneticField/detail/SmallObjectCache.hpp	`79.06% <ø> (-0.48%)`	:arrow_down:
.../include/Acts/Propagator/MultiEigenStepperLoop.hpp	`72.02% <0.00%> (ø)`
...ts/Propagator/detail/VolumeMaterialInteraction.hpp	`89.47% <ø> (ø)`
Core/include/Acts/Seeding/BinnedSPGroup.hpp	`0.00% <0.00%> (ø)`
Core/include/Acts/Surfaces/AnnulusBounds.hpp	`71.79% <ø> (ø)`
...de/Acts/TrackFinding/CombinatorialKalmanFilter.hpp	`32.33% <0.00%> (-0.26%)`	:arrow_down:
Core/include/Acts/Utilities/BinUtility.hpp	`46.53% <0.00%> (-1.55%)`	:arrow_down:
Core/include/Acts/TrackFitting/detail/GsfActor.hpp	`43.30% <33.33%> (ø)`
.../include/Acts/Material/InterpolatedMaterialMap.hpp	`43.47% <50.00%> (ø)`
Core/include/Acts/Utilities/KDTree.hpp	`57.93% <50.00%> (ø)`
... and 4 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

Aug 16 '22 13:08 codecov[bot]

Hm, this now seems to have a number of false negatives.

Sep 05 '22 11:09 paulgessinger

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.

Sep 06 '22 08:09 paulgessinger

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

Sep 06 '22 09:09 paulgessinger

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

Sep 06 '22 12:09 andiwand

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...

Sep 06 '22 12:09 benjaminhuth

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 find and parallel to execute clang-tidy on all the headers, dropping unavoidable compile errors, and merge everything together.

Sep 06 '22 12:09 paulgessinger

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 :smile:

Sep 06 '22 12:09 andiwand

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 ints, and then complaining that bool was auto-converted to int. That's clearly wrong, so I filtered this by disabling that specific warning for the header mode.

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.

Sep 06 '22 12:09 paulgessinger

Just one question maybe: There are these two files .clang-tidy and .clang-tidy-headers, and additional to that in the file cmake/ActsStaticAnalysis.cmake are also some options defined... Is some of this redundant or do I not understand correctly how this interacts?

Sep 06 '22 12:09 benjaminhuth

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 .clang-tidy so that editors (vim, vscode, etc) could give you clang-tidy warnings while editing. This is (sort of coincidentally) also the file that the job picks up when I run it manually on the header files in this PR. To disable the problematic warnings, I added the .clang-tidy-headers file and dropped them.

Do you have any other ideas how to solve this better?

Sep 06 '22 13:09 paulgessinger

Okay, that sounds reansonable. Let's merge then!

Sep 06 '22 13:09 benjaminhuth

should the cmake clang-tidy config and .clang-tidy always match? if so it might be a future improvement to read from / write to one of them

Sep 06 '22 14:09 andiwand

You mean to generate it in the CI before the clang-tidy job runs?

Sep 06 '22 14:09 paulgessinger

either generate .clang-tidy from by running cmake or reading .clang-tidy from cmake and use it for calling clang-tidy locally and in the ci

Sep 06 '22 14:09 andiwand

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 .clang-tidy would be a good option if we don't want to steer the the checks using cmake options (which is not done currently I think)...

Sep 06 '22 14:09 benjaminhuth

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.