21.65 KB,
patch

roc
:
review+
glandium
:
feedback+

Details  Diff  Splinter Review 
Created attachment 8693477 [details] [diff] [review] replaceConstants.hwithmath.h.patch Standard C and C++ do not define M_PI in math.h, but all our current toolchains do (if we ask nicely). glibc's math.h needs #define _GNU_SOURCE, which is implicitly #defined by g++ (but not gcc!). MSVC's math.h needs #define _USE_MATH_DEFINES. This patch: 1. Removes mfbt/Constants.h (added in bug 776429). 2. Replaces #include "mozilla/Constants.h" with #include <cmath> unless the file already included math.h. 3. #define _USE_MATH_DEFINES globally for MSVC. Caveats: * By reverting from Constant.h's M_PI to each platform's math.h, the precision of M_PI will differ across platforms as noted in bug 776429 comment 16. * Defining _USE_MATH_DEFINES globally causes a macro redefinition warningaserror in the media/libcubeb library. I suppressed the libcubeb warning in this patch, but I can submit a fix upstream. libcubeb is a standalone library maintained by :kinetik.
Comment 1•2 years ago


Comment on attachment 8693477 [details] [diff] [review] replaceConstants.hwithmath.h.patch Review of attachment 8693477 [details] [diff] [review]:  The inconsistency in the use of math.h vs. cmath is disconcerting. Apart from that, implementationwise, this looks sane. However, I'm not sure about the precision issue, and I'd rather get someone else's opinion on this, although there's also one bit of information lacking: what is the value of M_PI as provided by MSVC headers?
(Assignee)  
Comment 2•2 years ago


In files that #included Constants.h but not also math.h, I #included cmath instead of math.h because it seemed that cmath was used in our newer code (e.g. MFBT). cmath #includes math.h but replaces some macros with inline functions and namespaces. M_PI precision across platforms: MSVC: 3.14159265358979323846 (https://msdn.microsoft.com/enus/library/4hwaceh6.aspx) Linux: 3.14159265358979323846 (Linux glibc and Android bionic) OS X: 3.14159265358979323846264338327950288
Comment on attachment 8693477 [details] [diff] [review] replaceConstants.hwithmath.h.patch Review of attachment 8693477 [details] [diff] [review]:  In most places where we use M_PI we are also using trig functions and I think the precision of the trig functions is also platformdependent, so we don't lose anything by using M_PI.
Comment 5•2 years ago


bugherder 
https://hg.mozilla.org/mozillacentral/rev/a1bf9a99ec4b
Comment 7•2 years ago


bugherder 
https://hg.mozilla.org/mozillacentral/rev/051f3048f7e7
Description
•