HPCC-12708 Changes following review.

Signed-off-by: Gavin Halliday <gavin.halliday@lexisnexis.com>

system/jlib/jset.cpp

@@ -104,7 +104,7 @@ protected:
                     //Guaranteed a match at this point
                     //Returns one the index of the least significant 1-bit of testMask
                     //(testMask != 0) since that has been checked above (noMatchMask == 0)
-                    unsigned pos = countTrailingZeros(testMask);
+                    unsigned pos = countTrailingUnsetBits(testMask);
                     if (scninv)
                     {
                         bits_t t = ((bits_t)1)<<pos;
@@ -129,7 +129,7 @@ protected:
                 //Guaranteed a match at this point
                 //Returns one the index of the least significant 1-bit of testMask
                 //(testMask != 0) since that has been checked above (noMatchMask == 0)
-                unsigned pos = countTrailingZeros(testMask);
+                unsigned pos = countTrailingUnsetBits(testMask);
                 if (scninv)
                 {
                     bits_t t = ((bits_t)1)<<pos;

system/jlib/jset.hpp

@@ -26,8 +26,8 @@
 #include <intrin.h>
 #endif
 
-//Return the nunber of trailing zeros. Deliberately undefined if value == 0
-inline unsigned countTrailingZeros(unsigned value)
+//Return the number of trailing zeros. Deliberately undefined if value == 0
+inline unsigned countTrailingUnsetBits(unsigned value)
 {
     dbgassertex(value != 0);
 #if defined(__GNUC__)
@@ -49,8 +49,8 @@ inline unsigned countTrailingZeros(unsigned value)
 #endif
 }
 
-//Return the nunber of leading zeros. Deliberately undefined if value == 0
-inline unsigned countLeadingZeros(unsigned value)
+//Return the number of leading zeros. Deliberately undefined if value == 0
+inline unsigned countLeadingUnsetBits(unsigned value)
 {
     dbgassertex(value != 0);
 #if defined(__GNUC__)
@@ -58,9 +58,9 @@ inline unsigned countLeadingZeros(unsigned value)
 #elif defined (_WIN32)
     unsigned long index;
     _BitScanReverse(&index, value);
-    return (unsigned)index;
+    return (unsigned)((sizeof(unsigned)*8)-1 - index);
 #else
-    unsigned mask = 1U << 31;
+    unsigned mask = 1U << ((sizeof(unsigned)*8)-1);
     unsigned i;
     for (i=0; i < sizeof(unsigned)*8; i++)
     {
@@ -72,6 +72,29 @@ inline unsigned countLeadingZeros(unsigned value)
 #endif
 }
 
+//Return the number of bits including the first non-zero bit.  Undefined if value == 0
+inline unsigned getMostSignificantBit(unsigned value)
+{
+    dbgassertex(value != 0);
+#if defined(__GNUC__)
+    return (sizeof(unsigned)*8) - __builtin_clz(value);
+#elif defined (_WIN32)
+    unsigned long index;
+    _BitScanReverse(&index, value);
+    return (unsigned)index+1;
+#else
+    unsigned mask = 1U << ((sizeof(unsigned)*8)-1);
+    unsigned i;
+    for (i=0; i < sizeof(unsigned)*8; i++)
+    {
+        if (value & mask)
+            return sizeof(unsigned)*8-i;
+        mask = mask >> 1;
+    }
+    return 0;
+#endif
+}
+
 interface jlib_decl IBitSet : public IInterface 
 {
     virtual void set(unsigned n,bool val=true)      = 0;

testing/unittests/jlibtests.cpp

@@ -90,16 +90,30 @@ class JlibSetTest : public CppUnit::TestFixture
 {
 public:
     CPPUNIT_TEST_SUITE(JlibSetTest);
+        CPPUNIT_TEST(testBitsetHelpers);
         CPPUNIT_TEST(testSimple);
     CPPUNIT_TEST_SUITE_END();
 
 protected:
 
+    void testBitsetHelpers()
+    {
+        CPPUNIT_ASSERT_EQUAL(0U, countTrailingUnsetBits(1));
+        CPPUNIT_ASSERT_EQUAL(31U, countLeadingUnsetBits(1));
+        CPPUNIT_ASSERT_EQUAL(1U, getMostSignificantBit(1));
+        CPPUNIT_ASSERT_EQUAL(4U, countTrailingUnsetBits(0x110));
+        CPPUNIT_ASSERT_EQUAL(23U, countLeadingUnsetBits(0x110));
+        CPPUNIT_ASSERT_EQUAL(9U, getMostSignificantBit(0x110));
+        CPPUNIT_ASSERT_EQUAL(0U, countTrailingUnsetBits(0xFFFFFFFFU));
+        CPPUNIT_ASSERT_EQUAL(0U, countLeadingUnsetBits(0xFFFFFFFFU));
+        CPPUNIT_ASSERT_EQUAL(32U, getMostSignificantBit(0xFFFFFFFFU));
+    }
+
     void testSet1(bool initial, IBitSet *bs, unsigned start, unsigned numBits, bool setValue, bool clearValue)
     {
         unsigned end = start+numBits;
         if (initial)
-            bs->incl(start, end);
+            bs->incl(start, end-1);
         for (unsigned i=start; i < end; i++)
         {
             ASSERT(bs->test(i) == clearValue);
@@ -111,8 +125,10 @@ protected:
             ASSERT(bs->scan(i+1, setValue) == i+5);
             bs->set(i, clearValue);
             bs->set(i+5, clearValue);
+            //Clearing i+5 above may extend the set - so need to calculate the end carefully
+            unsigned last = i+5 < end ? end : i + 6;
             unsigned match1 = bs->scan(0, setValue);
-            ASSERT(match1 == initial ? -1 : end);
+            CPPUNIT_ASSERT_EQUAL((unsigned)(initial ? last : -1), match1);
 
             bs->invert(i);
             ASSERT(bs->test(i) == setValue);