I just hit an error in third party code that the clang plugin emits when a float is compared to itself for NaN detection. That error invites to use mozilla::IsNaN. That function comes from the C++-ification of a C macro that used to live under content/ all the way back to bug 278981, back when C++11 was not a thing.

It seems we should be able to replace it with the now C++ std equivalent, and the same for IsInfinite and probably others.

Of course, the clang-plugin error message should be adjusted too.

This is also necessary for upgrading gcc to gcc 10 (or maybe 11?). mozilla::IsNaN will make gcc issue a warning that will fail a warnings-as-errors build.

We have builds on automation with gcc 11, with warnings-as-errors, and they're not failing. https://treeherder.mozilla.org/jobs?repo=mozilla-central&searchStr=linux%2Cbuild%2Cgcc

(In reply to Steve Fink [:sfink] [:s:] from comment #1)

This is also necessary for upgrading gcc to gcc 10 (or maybe 11?). mozilla::IsNaN will make gcc issue a warning that will fail a warnings-as-errors build.

The warning could be #pragma'd off if it came to it.

The problem with the sanctioned C++ forms was miscompilation with various compilers. Wouldn't be hard to check if that's an issue still by just replacing the bodies of all the functions with calls to the standard functions, then let it sit awhile and see if regressions are reported.

Hm, I can't reproduce it locally right now. Perhaps it was the gcc that came with Fedora 33? I recently upgraded to 34, and now have gcc-11.1.1.

This came up because tcampbell ran into it a day or two ago when attempting to reproduce a jsapi-tests failure. (I think he's on Ubuntu, though, not Fedora.) Which is weird, come to think of it, because now that autospider.py runs via mach I would expect it to get the compiler from ~/.mozbuild/gcc.

We don't bootstrap gcc.

tcampbell says "./mach build + a mozconfig that said CC=gcc CXX=g++", and "my system gcc is 9.3".

I compiled the JS shell with gcc 9.3.0 (it's what get installed by mach hazards bootstrap). Still no warning.

Bleh. I will ignore this for now.

This looks easy enough to be a good first bug. Please unset it if anyone disagrees.

I'd like to work on this!

You're welcome to :)

This is my first bug, I've replaced all instances of mozilla::IsNaN with std::isnan, mozilla::IsInfinite with std::isinf, and mozilla::IsFinite with std::isfinite. I am unsure which review group(s) I should submit this to, considering how the changes are in many different modules, but they are also relatively inconsequential within each module, could I get some guidance? :')

Replaced mozilla::IsNaN with std::isnan, mozilla::IsInfinite with
std::isinf, and mozilla::IsFinite with std::isfinite. Also removed their
declarations from mfbt/FloatingPoint.h and replaced them with std
equivalents in mfbt/tests/TestFloatingPoint.cpp (although it'd probably
be cleaner to just get rid of the tests).

Also changed message in build/clang-plugin/NaNExprChecker.cpp to
advocate using std::isnan.

Late to the party, but I'd split the patches per the components for easier review. Including everything in a single patch often requires multiple reviews which can be frustrating. Splitting allows you to land one by one as the review proceeds.

Is this issue still active? I would love to contribute it to it as my first bug!!

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

I missed comment #13, yes it is active.

Depends on D173035

Depends on D173036

glandium what do you say, should we also remove mozilla::IsNegative()? My main concern is that it encapsulates a MOZ_ASSERT.

Depends on D173037

Depends on D173048

Depends on D173049

(In reply to Andi [:andi] from comment #19)

glandium what do you say, should we also remove mozilla::IsNegative()? My main concern is that it encapsulates a MOZ_ASSERT.

What would you replace it with?

Since we cannot move away from mozilla::IsNegative to std::signbit because the
first one doesn't accept a NaN we should transform our function to use std implementation.

Depends on D173050

Backed out for causing wasi bustages on Linux x64 opt.

• Backout link
• Push with failures - wasi bustages
• Failure Log
• Failure line: /builds/worker/checkouts/gecko/js/src/jsdate.cpp:544:8: error: use of undeclared identifier 'IsFinite'; did you mean 'isfinite'?

It seems there were two references t0 IsFinite that were bare with no mozilla:: so were missed by your search.

Meant to say 2 references is jsdate.cpp to IsFinite

https://hg.mozilla.org/mozilla-central/rev/458f942ecef8
https://hg.mozilla.org/mozilla-central/rev/e44ca4fc1195
https://hg.mozilla.org/mozilla-central/rev/3735e6d16998
https://hg.mozilla.org/mozilla-central/rev/e0fbc2d0840b
https://hg.mozilla.org/mozilla-central/rev/647666f69ada
https://hg.mozilla.org/mozilla-central/rev/0b66fb4d1382
https://hg.mozilla.org/mozilla-central/rev/9385659b1013