1228947 - Replace mfbt/Constants.h with math.h

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: replace-Constants.h-with-math.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 warning-as-error 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

[Mike Hommey [:glandium]]

Comment on attachment 8693477 [details] [diff] [review]
replace-Constants.h-with-math.h.patch

Review of attachment 8693477 [details] [diff] [review]:
-----------------------------------------------------------------

The inconsistency in the use of math.h vs. cmath is disconcerting. Apart from that, implementation-wise, 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?

Comment 2

[Chris Peterson [:cpeterson]]

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/en-us/library/4hwaceh6.aspx)
Linux: 3.14159265358979323846 (Linux glibc and Android bionic)
OS X:  3.14159265358979323846264338327950288

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8693477 [details] [diff] [review]
replace-Constants.h-with-math.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 platform-dependent, so we don't lose anything by using M_PI.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1bf9a99ec4b

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a1bf9a99ec4b

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/051f3048f7e7

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/051f3048f7e7