Audio.cpp: warning: explicit comparison with `NaN` in fast floating point mode

[illwieckz]

This is a warning I get with ICX 2023.1.0 (which is based on very recent LLVM clang):

src/engine/audio/Audio.cpp:86:20: warning: explicit comparison with NaN in fast floating point mode [-Wtautological-constant-compare]
        return not std::isnan(v[0]) and not std::isnan(v[1]) and not std::isnan(v[2]);
                   ^~~~~~~~~~~~~~~~
src/engine/audio/Audio.cpp:86:45: warning: explicit comparison with NaN in fast floating point mode [-Wtautological-constant-compare]
        return not std::isnan(v[0]) and not std::isnan(v[1]) and not std::isnan(v[2]);
                                            ^~~~~~~~~~~~~~~~
src/engine/audio/Audio.cpp:86:70: warning: explicit comparison with NaN in fast floating point mode [-Wtautological-constant-compare]
        return not std::isnan(v[0]) and not std::isnan(v[1]) and not std::isnan(v[2]);
                                                                     ^~~~~~~~~~~~~~~~
src/engine/audio/Audio.cpp:439:57: warning: explicit comparison with NaN in fast floating point mode [-Wtautological-constant-compare]
        if (slotNum < 0 or slotNum >= N_REVERB_SLOTS or std::isnan(ratio)) {
                                                        ^~~~~~~~~~~~~~~~~

If I'm right, -ffast-math enforces finite math, and then never emits NaN or Infinite.

If we expect to get NaN for some third-party library, we may want to not build Audio.cpp with finite math disabled, or, if the NaN values come from our code, rewrite that code to not return NaN.

[slipher]

Presumably the vectors come from the cgame. The engine must not make any assumptions on what comes out of there.

[illwieckz]

Presumably the vectors come from the cgame. The engine must not make any assumptions on what comes out of there.

Does that mean we can't use std::isnan? Or that we may still get NaN anyway?

[slipher]

It means that "fixing code that generates NaN" is not an option. The engine should be prepared for any value coming from a cgame trap call.

[illwieckz]

So the compiler wrongly assumes the data is produced by the same code processing it… right?

Do we know if std::isnan is expected to work in all cases? What if compiler developers decide to optimize it by always returning false when built with -ffast-math?

[slipher]

We could change it to isfinite. The compiler doesn't warn on that and I assume that infinite vectors don't need to be valid either

[illwieckz]

We also now get this warning in other parts of the code:

Unvanquished/daemon/src/common/Cvar.cpp:85:42: warning: use of infinity is undefined behavior due to the currently enabled floating-point options [-Wnan-infinity-disabled]
   85 |         if (Str::ToFloat(value, temp) && isfinite(temp)) {
      |                                          ^~~~~~~~~~~~~~

In file included from Unvanquished/src/shared/navgen/nav.cpp:41:
In file included from Unvanquished/src/shared/navgen/navgen.h:29:
In file included from Unvanquished/libs/recastnavigation/Detour/Include/DetourCommon.h:22:
Unvanquished/libs/recastnavigation/Detour/Include/DetourMath.h:19:46: warning: use of infinity is undefined behavior due to the currently enabled floating-point options [-Wnan-infinity-disabled]
   19 | inline bool dtMathIsfinite(float x) { return isfinite(x); }
      |                                              ^~~~~~~~~~~

So I guess changing it to isFinite will be wrong as well.

Edit: I get this with Clang 18.

[slipher]

Maybe we should turn off the finite-math-assuming flag in the engine for this reason. I think when writing the previous comments, I didn't understand how doing any operation on a NaN or infinity basically invokes undefined behavior with that flag enabled. It's not good that a naughty cgame could cause undefined behavior through any trap call with a float.