Replace mfbt/Constants.h with math.h

RESOLVED FIXED in Firefox 45

Status

()

Core
MFBT
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Created attachment 8693477 [details] [diff] [review]
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.
Attachment #8693477 - Flags: review?(mh+mozilla)
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?
Attachment #8693477 - Flags: review?(roc)
Attachment #8693477 - Flags: review?(mh+mozilla)
Attachment #8693477 - Flags: feedback+
(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/en-us/library/4hwaceh6.aspx)
Linux: 3.14159265358979323846 (Linux glibc and Android bionic)
OS X:  3.14159265358979323846264338327950288
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.
Attachment #8693477 - Flags: review?(roc) → review+

Comment 4

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

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a1bf9a99ec4b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Comment 6

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

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/051f3048f7e7
You need to log in before you can comment on or make changes to this bug.