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

build: Add -Wfloat-conversion to clang warnings #1676

Merged
merged 10 commits into from
Jan 17, 2023

Conversation

benjaminhuth
Copy link
Member

this seems at least in clang catch implicit float-to-bool conversion.

@benjaminhuth benjaminhuth added the Infrastructure Changes to build tools, continous integration, ... label Nov 15, 2022
@benjaminhuth benjaminhuth added this to the next milestone Nov 15, 2022
Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

lets see if that catches #1673 too

@paulgessinger
Copy link
Member

This is a good idea. Just needs the code fixes now 😄

@benjaminhuth
Copy link
Member Author

I would propose a Fix-a-thon for this :D

@benjaminhuth benjaminhuth added the 🚧 WIP Work-in-progress label Nov 22, 2022
@paulgessinger
Copy link
Member

I fixed all the conversion warnings with static_cast or more clever replacements.

@benjaminhuth
Copy link
Member Author

Would it be okay to merge this with the warning only enabled for clang @paulgessinger? Then we have at least these changes in for now.

@github-actions
Copy link

github-actions bot commented Jan 10, 2023

📊 Physics performance monitoring for c9cb63d

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

Vertexing

Vertexing vs. mu
IVF seeded

IVF truth_smeared

IVF truth_estimated

IVF orthogonal

Seeding

Seeding seeded

Seeding truth_estimated

Seeding orthogonal

CKF

CKF seeded

CKF truth_smeared

CKF truth_estimated

CKF orthogonal

Ambiguity resolution

seeded

Truth tracking (Kalman Filter)

Truth tracking

Truth tracking (GSF)

Truth tracking

@paulgessinger
Copy link
Member

I guess the only argument against would be that it's maybe unexpected to see a warning in the CI and then not locally. But to some extent this is already the case depending on which compiler you use.

@benjaminhuth benjaminhuth changed the title build: Add -Wconversion to compiler warnings build: Add -Wfloat-conversion to clang warnings Jan 11, 2023
@benjaminhuth benjaminhuth removed the 🚧 WIP Work-in-progress label Jan 11, 2023
@codecov
Copy link

codecov bot commented Jan 12, 2023

Codecov Report

Merging #1676 (c9cb63d) into main (70dc953) will decrease coverage by 0.05%.
The diff coverage is 70.73%.

@@            Coverage Diff             @@
##             main    #1676      +/-   ##
==========================================
- Coverage   49.83%   49.77%   -0.06%     
==========================================
  Files         406      406              
  Lines       22532    22521      -11     
  Branches    10280    10286       +6     
==========================================
- Hits        11228    11210      -18     
- Misses       4131     4132       +1     
- Partials     7173     7179       +6     
Impacted Files Coverage Δ
Core/include/Acts/Seeding/BinnedSPGroup.ipp 0.00% <0.00%> (ø)
Core/include/Acts/Seeding/SpacePointGrid.ipp 0.00% <0.00%> (ø)
...de/Acts/TrackFitting/detail/KLMixtureReduction.hpp 56.25% <ø> (ø)
Core/src/Material/VolumeMaterialMapper.cpp 8.52% <0.00%> (ø)
Core/src/Material/MaterialGridHelper.cpp 62.30% <70.83%> (-7.35%) ⬇️
Core/include/Acts/Geometry/SurfaceArrayCreator.hpp 42.85% <100.00%> (ø)
Core/include/Acts/Material/MaterialComposition.hpp 81.08% <100.00%> (ø)
Core/include/Acts/Surfaces/detail/FacesHelper.hpp 55.55% <100.00%> (ø)
...re/include/Acts/Surfaces/detail/VerticesHelper.hpp 54.54% <100.00%> (+1.42%) ⬆️
Core/include/Acts/Utilities/BinningData.hpp 69.23% <100.00%> (ø)
... and 7 more

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

@kodiakhq
Copy link
Contributor

kodiakhq bot commented Jan 16, 2023

This PR currently has a merge conflict. Please resolve this and then re-add the automerge label.

@kodiakhq kodiakhq bot merged commit f266387 into acts-project:main Jan 17, 2023
@paulgessinger paulgessinger modified the milestones: next, v23.0.0 Jan 18, 2023
@benjaminhuth benjaminhuth deleted the chore/add-wconversion branch February 22, 2023 07:43
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.

None yet

3 participants