Introduce llless(), and use it for llmin(), llmax().

Add tests to verify that llless() correctly handles signed <=> unsigned
comparison, which native "<" does not.

2 files changed, 119 insertions, 2 deletions

diff --git a/indra/llcommon/lldefs.h b/indra/llcommon/lldefs.h
index 2fbb26dc1a..ed075e8d96 100644
--- a/indra/llcommon/lldefs.h
+++ b/indra/llcommon/lldefs.h
@@ -28,6 +28,7 @@
 #define LL_LLDEFS_H
 
 #include "stdtypes.h"
+#include <cassert>
 #include <type_traits>
 
 // Often used array indices
@@ -169,6 +170,31 @@ constexpr U32   MAXADDRSTR      = 17;       // 123.567.901.345 = 15 chars + \0 +
 //   llclampb(a)     // clamps a to [0 .. 255]
 //
 
+// llless(d0, d1) safely compares d0 < d1 even if one is signed and the other
+// is unsigned. A simple (d0 < d1) expression converts the signed operand to
+// unsigned before comparing. If the signed operand is negative, that flips
+// the negative value to a huge positive value, producing the wrong answer!
+// llless() specifically addresses that case.
+template <typename T0, typename T1>
+inline bool llless(T0 d0, T1 d1)
+{
+    if constexpr (std::is_signed_v<T0> && ! std::is_signed_v<T1>)
+    {
+        // T0 signed, T1 unsigned: negative d0 is less than any unsigned d1
+        if (d0 < 0)
+            return true;
+    }
+    else if constexpr (! std::is_signed_v<T0> && std::is_signed_v<T1>)
+    {
+        // T0 unsigned, T1 signed: any unsigned d0 is greater than negative d1
+        if (d1 < 0)
+            return false;
+    }
+    // both T0 and T1 are signed, or both are unsigned, or both non-negative:
+    // straightforward comparison works
+    return d0 < d1;
+}
+
 // recursion tail
 template <typename T>
 inline auto llmax(T data)
@@ -180,7 +206,7 @@ template <typename T0, typename T1, typename... Ts>
 inline auto llmax(T0 d0, T1 d1, Ts... rest)
 {
     auto maxrest = llmax(d1, rest...);
-    return (d0 > maxrest)? d0 : maxrest;
+    return llless(maxrest, d0)? d0 : maxrest;
 }
 
 // recursion tail
@@ -194,12 +220,28 @@ template <typename T0, typename T1, typename... Ts>
 inline auto llmin(T0 d0, T1 d1, Ts... rest)
 {
     auto minrest = llmin(d1, rest...);
-    return (d0 < minrest) ? d0 : minrest;
+    return llless(d0, minrest) ? d0 : minrest;
 }
 
 template <typename A, typename MIN, typename MAX>
 inline A llclamp(A a, MIN minval, MAX maxval)
 {
+    // The only troublesome case is if A is unsigned and either minval or
+    // maxval is both signed and negative. Casting a negative number to
+    // unsigned flips it to a huge positive number, making this llclamp() call
+    // ineffective.
+    if constexpr (! std::is_signed_v<A>)
+    {
+        if constexpr (std::is_signed_v<MIN>)
+        {
+            assert(minval >= 0);
+        }
+        if constexpr (std::is_signed_v<MAX>)
+        {
+            assert(maxval >= 0);
+        }
+    }
+
     A aminval{ static_cast<A>(minval) }, amaxval{ static_cast<A>(maxval) };
     if ( a < aminval )
     {
diff --git a/indra/llcommon/tests/llcond_test.cpp b/indra/llcommon/tests/llcond_test.cpp
index f2a302ed13..7d1ac6edb9 100644
--- a/indra/llcommon/tests/llcond_test.cpp
+++ b/indra/llcommon/tests/llcond_test.cpp
@@ -19,6 +19,7 @@
 // other Linden headers
 #include "../test/lltut.h"
 #include "llcoros.h"
+#include "lldefs.h"                 // llless()
 
 /*****************************************************************************
 *   TUT
@@ -64,4 +65,78 @@ namespace tut
         cond.set_all(2);
         cond.wait_equal(3);
     }
+
+    template <typename T0, typename T1>
+    struct compare
+    {
+        const char* desc;
+        T0 lhs;
+        T1 rhs;
+        bool expect;
+
+        void test() const
+        {
+            // fails
+//          ensure_equals(desc, (lhs <  rhs), expect);
+            ensure_equals(desc, llless(lhs, rhs), expect);
+        }
+    };
+
+    template<> template<>
+    void object::test<3>()
+    {
+        set_test_name("comparison");
+        // Try to construct signed and unsigned variables such that the
+        // compiler can't optimize away the code to compare at runtime.
+        std::istringstream input("-1 10 20 10 20");
+        int minus1, s10, s20;
+        input >> minus1 >> s10 >> s20;
+        unsigned u10, u20;
+        input >> u10 >> u20;
+        ensure_equals("minus1 wrong", minus1, -1);
+        ensure_equals("s10 wrong", s10, 10);
+        ensure_equals("s20 wrong", s20, 20);
+        ensure_equals("u10 wrong", u10, 10);
+        ensure_equals("u20 wrong", u20, 20);
+        // signed < signed should always work!
+        compare<int, int> ss[] =
+            { {"minus1 < s10", minus1, s10, true},
+              {"s10 < s10", s10, s10, false},
+              {"s20 < s10", s20, s20, false}
+            };
+        for (const auto& cmp : ss)
+        {
+            cmp.test();
+        }
+        // unsigned < unsigned should always work!
+        compare<unsigned, unsigned> uu[] =
+            { {"u10 < u20", u10, u20, true},
+              {"u20 < u20", u20, u20, false},
+              {"u20 < u10", u20, u10, false}
+            };
+        for (const auto& cmp : uu)
+        {
+            cmp.test();
+        }
+        // signed < unsigned ??
+        compare<int, unsigned> su[] =
+            { {"minus1 < u10", minus1, u10, true},
+              {"s10 < u10", s10, u10, false},
+              {"s20 < u10", s20, u10, false}
+            };
+        for (const auto& cmp : su)
+        {
+            cmp.test();
+        }
+        // unsigned < signed ??
+        compare<unsigned, int> us[] =
+            { {"u10 < minus1", u10, minus1, false},
+              {"u10 < s10", u10, s10, false},
+              {"u10 < s20", u10, s20, true}
+            };
+        for (const auto& cmp : us)
+        {
+            cmp.test();
+        }
+    }
 } // namespace tut