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

Use a macro to set the DefaultPrecision #18

Conversation

elvisdukaj
Copy link
Contributor

DefaultPrecision is fixed to double, making it hard for this library to work with other frameworks using different precision. This PR allows to define the precision with a macro, and use double as default fallback for not breaking existing code.

@edrosten
Copy link
Owner

edrosten commented Jun 12, 2024 via email

@elvisdukaj
Copy link
Contributor Author

Thank you very much for the quick feedback! I am porting some old code to conan2 and internally we are"hacking" the source code to redefine typedef float DefaultPrecision. This value is used internally, and the code fails to build with the upstream version. I don't know the rationale of changing this value to float, but I can investigate.

I can leave with the hack for now, I applied a patch to the conan recipe to make it work like before.

I'm a little reluctant to introduce this macro, since it semi globally changes the meaning of code

We are already using this in production in multiple OSs (Windows, Linux, Android, macOS, iOS) and multiple architectures (x86_64, arm8). The default value anyway won't be changed but it will give the possibility to customize this from outside. Do you see something risky to do?

@edrosten
Copy link
Owner

Thinking about it, I do still think it's something that can be quite dangerous to use, and can set one up for trouble. On the other hand, sometime you need escape hatch (see the FORTRAN one below).

@edrosten edrosten self-requested a review June 23, 2024 08:55
@edrosten edrosten merged commit 2982ecc into edrosten:master Jun 23, 2024
1 check passed
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.

2 participants