Fixed regression in Imath::succf() and Imath::predf() when negative v…

…alues are given (AcademySoftwareFoundation#1013) Courtesy Christian Aguilera <[email protected]> The issue was introduced in OpenEXR 2.4.0 as part of c8a7f6a#diff-f09c17d3ab2fe5564e547c8461f1a43a6fe1109ebaf663a8463d1a9643c20ee0, where the type of a member of a union was changed from signed to unsigned, breaking comparison against > 0 a few lines below. Since OpenEXR 2.4.0, Imath::predf(-2) returns -1.9999998807907104 instead of -2.000000238418579. Test coverage has been added for this regression. Fixes AcademySoftwareFoundation#999 (for the 2.4 line). Signed-off-by: Cary Phillips <[email protected]>
cary-ilm · May 12, 2021 · 33b6ae2 · 33b6ae2
1 parent 4212416
commit 33b6ae2
Show file tree

Hide file tree

Showing 2 changed files with 58 additions and 8 deletions.
diff --git a/IlmBase/Imath/ImathFun.cpp b/IlmBase/Imath/ImathFun.cpp
@@ -54,7 +54,7 @@ succf (float f)
 
         u.i = 0x00000001;
     }
-    else if (u.i > 0)
+    else if (u.f > 0)
     {
         // Positive float, normalized or denormalized.
         // Incrementing the largest positive float
@@ -89,7 +89,7 @@ predf (float f)
 
         u.i = 0x80000001;
     }
-    else if (u.i > 0)
+    else if (u.f > 0)
     {
         // Positive float, normalized or denormalized.
 

diff --git a/IlmBase/ImathTest/testFun.cpp b/IlmBase/ImathTest/testFun.cpp
@@ -38,6 +38,9 @@
 
 #include <testFun.h>
 #include "ImathFun.h"
+#if __cplusplus >= 202002L
+#    include <bit>
+#endif
 #include <iostream>
 #include <assert.h>
 #include <stdio.h>
@@ -51,9 +54,24 @@ using namespace std;
     typedef long long unsigned int Int64;
 #endif
 
+#if __cplusplus < 202002L
+    template <typename To, typename From>
+    static inline To
+    bit_cast (From from)
+    {
+        static_assert (sizeof (From) == sizeof (To), "Type sizes do not match");
+        union
+        {
+            From f;
+            To   t;
+        } u;
+        u.f = from;
+        return u.t;
+    }
+#endif
 
 void
-testf (float f)
+testf (float f, bool changeExpected = true)
 {
     printf ("\n");
 
@@ -67,11 +85,25 @@ testf (float f)
     printf ("pf %.9g\n", pf);
     printf ("spf %.9g\n", spf);
     printf ("psf %.9g\n", psf);
+
+    fflush (stdout);
+
+    if (changeExpected)
+    {
+        assert (pf < f);
+        assert (f < sf);
+    }
+    else
+    {
+        // No bit change expected if input was inf or NaN
+        assert (bit_cast<unsigned> (pf) == bit_cast<unsigned> (f));
+        assert (bit_cast<unsigned> (sf) == bit_cast<unsigned> (f));
+    }
 }
 
 
 void
-testd (double d)
+testd (double d, bool changeExpected = true)
 {
     printf ("\n");
 
@@ -85,6 +117,20 @@ testd (double d)
     printf ("pd %.18lg\n", pd);
     printf ("spd %.18lg\n", spd);
     printf ("psd %.18lg\n", psd);
+
+    fflush (stdout);
+
+    if (changeExpected)
+    {
+        assert (pd < d);
+        assert (d < sd);
+    }
+    else
+    {
+        // No bit change expected if input was inf or NaN
+        assert (bit_cast<Int64> (pd) == bit_cast<Int64> (d));
+        assert (bit_cast<Int64> (sd) == bit_cast<Int64> (d));
+    }
 }
 
 
@@ -188,9 +234,11 @@ testFun ()
 
     union {float f; int i;} u;
     u.i = 0x7f800000; //  inf
-    testf (u.f);
+    testf (u.f, false);
+    u.i = 0xff800000; // -inf
+    testf (u.f, false);
     u.i = 0x7f800001; //  nan
-    testf (u.f);
+    testf (u.f, false);
     u.i = 0x7f7fffff; //  FLT_MAX
     testf (u.f);
     u.i = 0xff7fffff; // -FLT_MAX
@@ -206,9 +254,11 @@ testFun ()
 
     union {double d; Int64 i;} v;
     v.i = 0x7ff0000000000000ULL; //  inf
-    testd (v.d);
+    testd (v.d, false);
+    v.i = 0xfff0000000000000ULL; // -inf
+    testd (v.d, false);
     v.i = 0x7ff0000000000001ULL; //  NAN
-    testd (v.d);
+    testd (v.d, false);
     v.i = 0x7fefffffffffffffULL; //  FLT_MAX
     testd (v.d);
     v.i = 0xffefffffffffffffULL; // -FLT_MAX