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

ci: Add clang-tidy check for &&, || and ! instead of and, or and not #2569

Merged
merged 26 commits into from
Oct 31, 2023

Conversation

paulgessinger
Copy link
Member

No description provided.

@paulgessinger paulgessinger added this to the next milestone Oct 23, 2023
@github-actions github-actions bot added the Infrastructure Changes to build tools, continous integration, ... label Oct 23, 2023
@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

Merging #2569 (3f0d9af) into main (fbc3bd8) will decrease coverage by 0.01%.
The diff coverage is 27.72%.

@@            Coverage Diff             @@
##             main    #2569      +/-   ##
==========================================
- Coverage   49.55%   49.54%   -0.01%     
==========================================
  Files         472      472              
  Lines       26724    26719       -5     
  Branches    12318    12313       -5     
==========================================
- Hits        13244    13239       -5     
  Misses       4748     4748              
  Partials     8732     8732              
Files Coverage Δ
Core/include/Acts/Geometry/CuboidVolumeBounds.hpp 62.06% <100.00%> (ø)
Core/include/Acts/Geometry/Extent.hpp 90.90% <ø> (ø)
...re/include/Acts/Geometry/SurfaceBinningMatcher.hpp 42.85% <100.00%> (ø)
Core/include/Acts/Propagator/AbortList.hpp 83.33% <ø> (ø)
Core/include/Acts/Propagator/ActionList.hpp 100.00% <ø> (ø)
Core/include/Acts/Propagator/DirectNavigator.hpp 75.71% <100.00%> (ø)
...e/include/Acts/Propagator/StepperExtensionList.hpp 64.58% <ø> (ø)
Core/include/Acts/Surfaces/CylinderBounds.hpp 68.75% <100.00%> (ø)
Core/include/Acts/Surfaces/DiamondBounds.hpp 62.50% <100.00%> (ø)
Core/include/Acts/Surfaces/TrapezoidBounds.hpp 73.91% <100.00%> (ø)
... and 106 more

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

@paulgessinger
Copy link
Member Author

Fixes are ready but I want to see the CI fail first.

@benjaminhuth
Copy link
Member

But why would we want to forbid and or and not?
I actually like them

@paulgessinger
Copy link
Member Author

paulgessinger commented Oct 23, 2023

As with everything, it's a matter of taste I find it very confusing, and it's bringing one of the worst parts of python syntax to C++.

And the worst imo is having both styles in the same codebase.

At the very least, we should then also switch the rest of the operators, like != to not_eq, & to bitand, | to bitor and ~ to compl. 😆

@andiwand
Copy link
Contributor

I also have a rather strong opinion of not using and / or / not because to me it feels like non-C++

but more importantly I think it should be consistent across the codebase

@paulgessinger
Copy link
Member Author

@andiwand We all have keyboards with keys for punctuation marks nowadays, right? 😅

@benjaminhuth
Copy link
Member

@andiwand We all have keyboards with keys for punctuation marks nowadays, right? 😅

Do we? 😆

But I agree, not_eq is terrible indeed. Few years ago I would have said the same about and, or and not but then python came...

However, I actually would argue to keep at least not as alternative to ! because in some cases I think that actually improves readability... If I quickly scan code with

if( !longVariableName )

vs.

if( not longVariableName )

the latter one is better readable I think.

@paulgessinger paulgessinger marked this pull request as ready for review October 24, 2023 08:42
andiwand
andiwand previously approved these changes Oct 30, 2023
@paulgessinger
Copy link
Member Author

Needed a few more fixes unfortunately, @andiwand.

andiwand
andiwand previously approved these changes Oct 31, 2023
@kodiakhq kodiakhq bot merged commit 1f6dd58 into acts-project:main Oct 31, 2023
53 checks passed
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Oct 31, 2023
@paulgessinger paulgessinger modified the milestones: next, v31.0.0 Nov 6, 2023
@paulgessinger paulgessinger removed the Fails Athena tests This PR causes a failure in the Athena tests label Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clustering Component - Core Affects the Core module Component - Examples Affects the Examples module Component - Fatras Affects the Fatras module Component - Plugins Affects one or more Plugins Event Data Model Infrastructure Changes to build tools, continous integration, ... Seeding Track Finding Track Fitting Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants