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

chore: clang tidy headers #1662

Merged
merged 77 commits into from
Nov 16, 2022
Merged

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Nov 9, 2022

fix clang tidy for header files. also includes fixes for clang-tidy warnings from @paulgessinger

replacement for #1440

@codecov
Copy link

codecov bot commented Nov 9, 2022

Codecov Report

Merging #1662 (4ee5a16) into main (b018119) will increase coverage by 0.05%.
The diff coverage is 58.57%.

@@            Coverage Diff             @@
##             main    #1662      +/-   ##
==========================================
+ Coverage   48.53%   48.58%   +0.05%     
==========================================
  Files         383      382       -1     
  Lines       21004    21059      +55     
  Branches     9649     9644       -5     
==========================================
+ Hits        10194    10232      +38     
- Misses       4130     4147      +17     
  Partials     6680     6680              
Impacted Files Coverage Δ
Core/include/Acts/EventData/MultiTrajectory.hpp 69.41% <ø> (ø)
...e/include/Acts/EventData/VectorMultiTrajectory.hpp 58.51% <ø> (ø)
Core/include/Acts/Geometry/BoundarySurfaceT.hpp 58.33% <ø> (ø)
Core/include/Acts/Geometry/ConeVolumeBounds.hpp 50.00% <ø> (ø)
...ore/include/Acts/Geometry/CylinderVolumeBounds.hpp 49.15% <ø> (ø)
Core/include/Acts/Geometry/SurfaceArrayCreator.hpp 42.85% <ø> (ø)
...re/include/Acts/Geometry/TrapezoidVolumeBounds.hpp 16.66% <ø> (ø)
...ude/Acts/MagneticField/detail/SmallObjectCache.hpp 79.06% <ø> (-0.48%) ⬇️
...ore/include/Acts/Material/VolumeMaterialMapper.hpp 42.85% <ø> (ø)
...re/include/Acts/Propagator/CovarianceTransport.hpp 0.00% <ø> (ø)
... and 88 more

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

@github-actions
Copy link

github-actions bot commented Nov 9, 2022

📊 Physics performance monitoring for 4ee5a16

Full report
CKF: seeded, truth smeared, truth estimated
IVF: seeded, truth smeared, truth estimated
Ambiguity resolution
Truth tracking (Kalman Filter)
Truth tracking (GSF)

Vertexing

Vertexing vs. mu
IVF seeded

IVF truth_smeared

IVF truth_estimated

CKF

CKF seeded

CKF truth_smeared

CKF truth_estimated

Ambiguity resolution

seeded

Truth tracking (Kalman Filter)

Truth tracking

Truth tracking (GSF)

Truth tracking

@andiwand
Copy link
Contributor Author

andiwand commented Nov 9, 2022

@paulgessinger looks like cmake refuses to compile header files

@paulgessinger
Copy link
Member

Did you check if the headers end up in compile_commands.json?

@andiwand
Copy link
Contributor Author

andiwand commented Nov 9, 2022

Did you check if the headers end up in compile_commands.json?

yeah I check - sadly they don't show up. I guess cmake is refusing to build these files because of their extension

@paulgessinger
Copy link
Member

@andiwand
Copy link
Contributor Author

andiwand commented Nov 9, 2022

Maybe we need something like: https://stackoverflow.com/questions/59716373/compile-headers-for-errors-using-cmake ?

hmm but don't we include our headers in some of our source files anyway?

@paulgessinger
Copy link
Member

Maybe some we don't? I don't actually fully understand this tbh.

@andiwand
Copy link
Contributor Author

Maybe some we don't? I don't actually fully understand this tbh.

so the source file for a header file does not seem to do the trick either... I need to check how this is supposed to work with header files again

@andiwand andiwand marked this pull request as ready for review November 11, 2022 14:23
@andiwand andiwand added the 🚧 WIP Work-in-progress label Nov 11, 2022
@andiwand andiwand added this to the next milestone Nov 11, 2022
@andiwand
Copy link
Contributor Author

looks like this works now. https://gitlab.cern.ch/acts/ci-bridge/-/jobs/25840196 is failing because of the html report generation

@andiwand andiwand removed the 🚧 WIP Work-in-progress label Nov 16, 2022
@andiwand
Copy link
Contributor Author

this is passing now @paulgessinger @benjaminhuth if you want to take a look. I think it would be good to get this in soon to prevent merge conflicts as this is touching a lot of code in different places

Copy link
Member

@benjaminhuth benjaminhuth left a comment

Choose a reason for hiding this comment

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

Let's get this in!

@kodiakhq kodiakhq bot merged commit 85a6775 into acts-project:main Nov 16, 2022
@andiwand andiwand deleted the clang-tidy-headers branch November 16, 2022 11:48
@paulgessinger paulgessinger modified the milestones: next, v22.0.0 Dec 21, 2022
CarloVarni pushed a commit to CarloVarni/acts that referenced this pull request Dec 22, 2022
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]>
kodiakhq bot pushed a commit that referenced this pull request Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Changes to build tools, continous integration, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants