Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: froydnj, Unassigned)

Tracking

unspecified
mozilla23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

6 years ago
FloatingPoint.h claims that it can't use any C++ features because there are clients who want to include it from C.  grepping through the tree, however, indicates:

1. We are not including it from any C source files; and
2. Any header files that include it are already using C++ features.

(The last is not quite true for a few JS header files, but since jsapi.h includes it and is C++, I figure it is a moot point.)
Posted patch Possible partial patch (obsolete) — Splinter Review
I did some free-time hackwork on this awhile back but never really finished things off completely.  This is incomplete and almost certainly doesn't build, but it probably gets a bit of the work done.

We might also consider replacing MOZ_DOUBLE_NaN() with std::numeric_limits<double>::quiet_NaN().  But note that historically there have been PGO issues with most ways of computing a NaN value, so extreme caution is warranted with whatever means are attempted.
Reporter

Comment 2

6 years ago
OK, I have updated your patch so it builds on x86-64 Linux.

What is/was your preference wrt MOZ_DOUBLE_NaN?  Should it simply become mozilla::UnspecificNaN or just continue to live on as MOZ_DOUBLE_NaN?
Flags: needinfo?(jwalden+bmo)
UnspecifiedNaN or something seems better.  The macro-like names were just because that was how it'd been done before, not actually desirable names.
Flags: needinfo?(jwalden+bmo)
Reporter

Comment 4

6 years ago
Here's a compiling version on x86-64 Linux; I think it catches the interesting
x86-only cases in the JS engine too.

The biggest change is having to export DoubleExponentBias and
DoubleExponentShift due to uses in the JS engine.  I'm not really
motivated to comment on whether those exports could be avoided
currently, so I went for the easiest solution.
Attachment #733421 - Attachment is obsolete: true
Attachment #745935 - Flags: review?(jwalden+bmo)
Comment on attachment 745935 [details] [diff] [review]
rewrite FloatingPoint.h to be C++-only instead of C-compatible C++

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

For the js/src/*.cpp changes that use mozilla::Foo, we should add |using mozilla::Foo;| to each file (underneath any using-namespaces, alphabetized with other |using mozilla::Foo|s) and kill off the mozilla::.  See json.cpp for an example.  I think I planned to do that after finishing off the whole set of conversions, but as I never got that far, I never undertook that set of changes.  (js/src headers, of course, should still use mozilla::Foo.)

r=me if you adjust the two actually-wrong changes noted in hunk-comments.  (Yes, I checked every conversion.  :-) )

::: content/html/content/src/HTMLInputElement.cpp
@@ +4993,1 @@
>      step = GetDefaultStep();

IsFinite means not NaN and not +Infinity and not -Infinity.
IsInfinite means is +Infinity or -Infinity.

So !MOZ_DOUBLE_IS_FINITE(step) means step is NaN, is +Infinity, or is -Infinity.
And IsInfinite(step) means step is +Infinity or is -Infinity.

Maybe this change is warranted, maybe not, I didn't look to HTML specs to see.  But it shouldn't be made here -- followup bug at best, if this is an actual bug.

::: content/media/webaudio/blink/DynamicsCompressorKernel.cpp
@@ +40,2 @@
>  using mozilla::dom::WebAudioUtils;
> +using mozilla::IsNaN;

Hmm, I imagine alphabetical here would put IsInfinite second, not first.

::: js/src/builtin/Intl.cpp
@@ +1435,1 @@
>          x = 0.0;

Uh, this is wrong.  This should be IsNegativeZero!

::: mfbt/FloatingPoint.h
@@ +149,5 @@
>     * The exponent component of a double is an unsigned number, biased from its
>     * actual value.  Subtract the bias to retrieve the actual exponent.
>     */
> +  return int_fast16_t((pun.u & detail::DoubleExponentBits) >> DoubleExponentShift) -
> +         DoubleExponentBias;

This is |signed - unsigned|, which promotes to |unsigned - unsigned|, which has type |unsigned|.  I guess add another signed cast around DoubleExponentBias here to fix this; in all the other cases the unsigned type makes sense, so we shouldn't just make it signed.
Attachment #745935 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/cec949998373
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.