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

Replaces global pi value with std::numbers::pi_v<T> #68

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cugone
Copy link

@cugone cugone commented Feb 9, 2024

Fixes #67

This replaces the global double-only pi variable with std::numbers::pi_v<T> from the <numbers> header. As a bonus it plays nice with templates and can now be represented as both a double and float depending on the template argument type instead of implicitly being cast to a float during multiplication operations.

@OneLoneCoder
Copy link
Owner

Hmmm, firstly it ruins my awesome joke :( . Secondly, I'm unconvinced it's an improvement - it's wordier and less clear. Just because the std has something, doesn't always mean it should. However I'm fair and quite malleable, convince me and others it's better and i'll merge it.

  • I'm unconvinced about the casting, I would suspect that's resolved quite happily at compile time.
  • I'm unconvinced it's clearer, keep in mind the audience of this library.

@cugone
Copy link
Author

cugone commented Feb 18, 2024

https://godbolt.org/z/7eT476ocM

I had to convince myself as well. They produce the same assembly output.

The clarity argument can be mitigated:

  • It's internal, at least for now.
    • "Excessive" namespace qualification can be mitigated via using std::numbers and the aliases provided by the numbers library. It defaults to double though, hence the pi_v<T> cruft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No need for global pi value. The <numbers> header exists in C++20.
2 participants