From 8de8daca601dc85e2b73687856f0a321016b4463 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 6 Sep 2024 12:31:24 -0400 Subject: Introduce llless(), and use it for llmin(), llmax(). Add tests to verify that llless() correctly handles signed <=> unsigned comparison, which native "<" does not. --- indra/llcommon/lldefs.h | 46 +++++++++++++++++++++- indra/llcommon/tests/llcond_test.cpp | 75 ++++++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 2 deletions(-) (limited to 'indra') 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 #include // 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 +inline bool llless(T0 d0, T1 d1) +{ + if constexpr (std::is_signed_v && ! std::is_signed_v) + { + // T0 signed, T1 unsigned: negative d0 is less than any unsigned d1 + if (d0 < 0) + return true; + } + else if constexpr (! std::is_signed_v && std::is_signed_v) + { + // 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 inline auto llmax(T data) @@ -180,7 +206,7 @@ template 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 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 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) + { + if constexpr (std::is_signed_v) + { + assert(minval >= 0); + } + if constexpr (std::is_signed_v) + { + assert(maxval >= 0); + } + } + A aminval{ static_cast(minval) }, amaxval{ static_cast(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 + 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 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 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 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 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 -- cgit v1.2.3