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

refactor: Clang tidy fixes #1308

Merged
merged 9 commits into from
Jul 15, 2022
Merged

Conversation

paulgessinger
Copy link
Member

This PR adds fixes for

  • readability-container-size-empty
  • modernize-use-override
  • modernize-use-equals-default
  • readability-implicit-bool-cast
  • readability-implicit-bool-conversion

because they're fairly easy to do, and can be problematic.

Also improves the reporting. #1306 should be merged first.

@paulgessinger paulgessinger added this to the next milestone Jul 8, 2022
@paulgessinger paulgessinger changed the title Clang tidy fixes refactor: Clang tidy fixes Jul 8, 2022
@codecov
Copy link

codecov bot commented Jul 8, 2022

Codecov Report

Merging #1308 (9f0faad) into main (9f03bc9) will decrease coverage by 0.00%.
The diff coverage is 33.33%.

❗ Current head 9f0faad differs from pull request most recent head 9f4de8c. Consider uploading reports for the commit 9f4de8c to get more accurate results

@@            Coverage Diff             @@
##             main    #1308      +/-   ##
==========================================
- Coverage   47.45%   47.44%   -0.01%     
==========================================
  Files         375      375              
  Lines       19778    19779       +1     
  Branches     9273     9274       +1     
==========================================
  Hits         9385     9385              
  Misses       4010     4010              
- Partials     6383     6384       +1     
Impacted Files Coverage Δ
Core/src/EventData/PrintParameters.cpp 25.71% <0.00%> (ø)
Core/src/Geometry/TrackingGeometry.cpp 45.94% <0.00%> (ø)
Core/src/Geometry/TrackingVolume.cpp 43.45% <0.00%> (-0.14%) ⬇️
Core/src/Geometry/Layer.cpp 57.57% <33.33%> (ø)
Core/src/Visualization/GeometryView3D.cpp 32.85% <50.00%> (ø)
Core/src/Geometry/GeometryIdentifier.cpp 61.53% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Contributor

@Corentin-Allaire Corentin-Allaire left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for adding this ! Do I understand properly that the rules that have been added will now be checked by clang_tidy ?

Unrelated comment but looking at the modification I noticed in multiple place in the code we use and instead &&, could/should we add some option to clang_tidy to standardize this ?

.clang-tidy Outdated Show resolved Hide resolved
@paulgessinger
Copy link
Member Author

Hey @Corentin-Allaire, indeed the CI should now enforce these rules.

I'm not aware it's possible in clang-tidy to flag && vs and etc. I don't think clang-format can do it either.

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.

I didn't even know that not, and, or exist in C/C++. had to google to see that they are a relic of C. not sad to seem them go 😄

@Corentin-Allaire
Copy link
Contributor

I didn't even know that not, and, or exist in C/C++. hat to google to see that they are a relic of C. not sad to seem them go 😄

You mean ! sad ;) They have always been part of the standard (apparently the reason being that some weird keyboard don't have those symbols). They have ton al alternative symbole in C++ in the same style you can replace { } by <% %> if you want to mess with people :)

@kodiakhq kodiakhq bot merged commit c6fb62f into acts-project:main Jul 15, 2022
@paulgessinger paulgessinger modified the milestones: next, v19.5.0 Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants