-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[VecOps] Use better solution to pick up M_PI on windows #3641
Conversation
Starting build on |
#include <math.h> | ||
#else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping both #include <cmath>
and #include <math.h>
should not hurt (I think)
@phsft-bot build |
Starting build on |
Build failed on ROOT-performance-centos7-multicore/default. Failing tests: |
Build failed on mac1014/cxx17. Warnings:
Failing tests: |
Build failed on windows10/default. |
Build failed on ROOT-fedora27/noimt. Failing tests: |
Build failed on ROOT-ubuntu18.04-i386/cxx14. Errors:
|
Starting build on |
Build failed on ROOT-ubuntu18.04-i386/cxx14. Warnings:
Failing tests: |
Starting build on |
Build failed on ROOT-ubuntu18.04-i386/cxx14. Warnings:
Failing tests:
|
Many thanks to @bellenot !
Let's check whether the PR builds are happy with this.
Remaining issue with this: Is it ok to include
math.h
orcmath
(or even both?) based on the platform? Could this interfere with picking up the overloads for the fast math functions sincemath.h
andcmath
define their API in different namespaces?@lmoneta You have insights into this? The PR works but still, I feel a little uncomfortable since I do not fully understand the impact of the different includes.