Fix bug in Bits<T>::get / set

Summary:
Everything worked except for getting properly aligned full blocks
because 1ULL << 64 is invalid (the shift amount must be strictly less than
the value size in bits)

Test Plan: test added

Reviewed By: philipp@fb.com

FB internal diff: D657800

diff --git a/folly/experimental/Bits.h b/folly/experimental/Bits.h
index 214bd66a27bd4cd088abd6607aac1c2a8088beb4..fd97ddd7723492ee0195c1008e0cf88c24001334 100644 (file)
--- a/folly/experimental/Bits.h
+++ b/folly/experimental/Bits.h
@@ -156,7 +156,12 @@ struct Bits {
   // (bitStart < sizeof(T) * 8, bitStart + count <= sizeof(T) * 8)
   static UnderlyingType innerGet(const T* p, size_t bitStart, size_t count);
 
+  static constexpr UnderlyingType zero = UnderlyingType(0);
   static constexpr UnderlyingType one = UnderlyingType(1);
+
+  static constexpr UnderlyingType ones(size_t count) {
+    return count < bitsPerBlock ? (one << count) - 1 : ~zero;
+  }
 };
 
 template <class T, class Traits>
@@ -180,8 +185,6 @@ template <class T, class Traits>
 inline void Bits<T, Traits>::set(T* p, size_t bitStart, size_t count,
                                  UnderlyingType value) {
   assert(count <= sizeof(UnderlyingType) * 8);
-  assert(count == sizeof(UnderlyingType) ||
-         (value & ~((one << count) - 1)) == 0);
   size_t idx = blockIndex(bitStart);
   size_t offset = bitOffset(bitStart);
   if (offset + count <= bitsPerBlock) {
@@ -217,7 +220,7 @@ inline void Bits<T, Traits>::innerSet(T* p, size_t offset, size_t count,
                                       UnderlyingType value) {
   // Mask out bits and set new value
   UnderlyingType v = Traits::loadRMW(*p);
-  v &= ~(((one << count) - 1) << offset);
+  v &= ~(ones(count) << offset);
   v |= (value << offset);
   Traits::store(*p, v);
 }
@@ -225,7 +228,7 @@ inline void Bits<T, Traits>::innerSet(T* p, size_t offset, size_t count,
 template <class T, class Traits>
 inline auto Bits<T, Traits>::innerGet(const T* p, size_t offset, size_t count)
   -> UnderlyingType {
-  return (Traits::load(*p) >> offset) & ((one << count) - 1);
+  return (Traits::load(*p) >> offset) & ones(count);
 }
 
 template <class T, class Traits>

diff --git a/folly/experimental/test/BitsTest.cpp b/folly/experimental/test/BitsTest.cpp
index 83dc8d06674470b24988543ce0e6df171ccb8be2..d01510314fa5fb1c6d6927df03d94cbb9a5da4f9 100644 (file)
--- a/folly/experimental/test/BitsTest.cpp
+++ b/folly/experimental/test/BitsTest.cpp
@@ -148,6 +148,7 @@ void runMultiBitTest64() {
   auto load = detail::BitsTraits<T>::load;
   T buf[] = {0x123456789abcdef0, 0x13579bdf2468ace0};
 
+  EXPECT_EQ(0x123456789abcdef0, load(Bits<T>::get(buf, 0, 64)));
   EXPECT_EQ(0xf0, load(Bits<T>::get(buf, 0, 8)));
   EXPECT_EQ(0x89abcdef, load(Bits<T>::get(buf, 4, 32)));
   EXPECT_EQ(0x189abcdef, load(Bits<T>::get(buf, 4, 33)));
@@ -156,6 +157,9 @@ void runMultiBitTest64() {
   EXPECT_EQ(0xd5555555, load(Bits<T>::get(buf, 4, 32)));
   EXPECT_EQ(0x1d5555555, load(Bits<T>::get(buf, 4, 33)));
   EXPECT_EQ(0xd55555550, load(Bits<T>::get(buf, 0, 36)));
+
+  Bits<T>::set(buf, 0, 64, 0x23456789abcdef01);
+  EXPECT_EQ(0x23456789abcdef01, load(Bits<T>::get(buf, 0, 64)));
 }
 
 TEST(Bits, MultiBit64) {