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 check for mathematical constants (no macros) #3774

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

AJPfleger
Copy link
Contributor

@AJPfleger AJPfleger commented Oct 23, 2024

The firstly intended clang tidy check modernize-use-std-numbers is not suited for us:

  • it doesn't flag macros like M_PI
  • it flags values in our tables that get close to constants, which have a different basis.

benefit would have been to detect cases like static_cast<float>(std::numbers::pi) instead of std::numbers::pi_v<float> or usages of std::sqrt(2.) instead of std::numbers::sqrt2.

The updated test now checks for the classical M_* macros that could be accidentally used.

blocked:

@AJPfleger AJPfleger added the 🚧 WIP Work-in-progress label Oct 23, 2024
@AJPfleger AJPfleger added this to the next milestone Oct 23, 2024
@github-actions github-actions bot added the Infrastructure Changes to build tools, continous integration, ... label Oct 23, 2024
Copy link

github-actions bot commented Oct 23, 2024

📊: Physics performance monitoring for 6614e7d

Full contents

physmon summary

@AJPfleger AJPfleger marked this pull request as draft October 23, 2024 12:52
@AJPfleger AJPfleger changed the title ci: Add clang-tidy check for mathematical constants ci: Add check for mathematical constants (no macros) Oct 24, 2024
@AJPfleger AJPfleger added 🛑 blocked This item is blocked by another item and removed 🚧 WIP Work-in-progress labels Oct 24, 2024
Copy link

sonarcloud bot commented Oct 24, 2024

@CarloVarni
Copy link
Collaborator

Hi @AJPfleger Is this test simply complaining or is it also trying to replace the code? In the latter case I have some concern since you may have to decide between (e.g.) std::numbers::pi_v<float> and std::numbers::pi_v<double> and std::numbers::pi

@AJPfleger
Copy link
Contributor Author

It has a way to fix misused macros. As everything in ACTS, no changes are made without a proper PR. Since we don't have any cases of the macros anymore, they could only be introduced over new PRs.

The author could attempt a quick fix using --fix. If it uses the wrong type, it would then need to slip by the author and the reviewer. In that case it is still a double, which would be the same type as the macro, which would also have slipped through.

So, I don't really see a worsening of the situation.

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, ... 🛑 blocked This item is blocked by another item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants