858131 - C++-ify FloatingPoint.h

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Nathan Froyd [:froydnj]]

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.)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Possible partial patch

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.

Comment 2

[Nathan Froyd [:froydnj]]

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?

Comment 3

[Jeff Walden [:Waldo]]

UnspecifiedNaN or something seems better.  The macro-like names were just because that was how it'd been done before, not actually desirable names.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: rewrite FloatingPoint.h to be C++-only instead of C-compatible C++

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.

Comment 5

[Jeff Walden [:Waldo]]

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.

Comment 6

[Nathan Froyd [:froydnj]]

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

Comment 7

[Ed Morley [:emorley]]

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