[llvm] r345772 - [adt] SparseBitVector::test() should be const

[Daniel Sanders via llvm-commits]

Author: dsanders
Date: Wed Oct 31 13:05:32 2018
New Revision: 345772

URL: http://llvm.org/viewvc/llvm-project?rev=345772&view=rev
Log:
[adt] SparseBitVector::test() should be const

Summary:
Re-worked SparseBitVector's most-recently-used-word caching (CurrElementIter)
such that SparseBitVector::test() can be made const. This came up when
attempting to test individual bits in a SparseBitVector which was a member of a
const object.

The cached iterator has no bearing on the externally visible state, it's merely
a performance optimization. Therefore it has been made mutable and
FindLowerBound() has been split into a const and non-const function
(FindLowerBound/FindLowerBoundConst) for the const/non-const
interfaces.

Reviewers: rtereshin

Reviewed By: rtereshin

Subscribers: rtereshin, dexonsmith, kristina, llvm-commits

Differential Revision: https://reviews.llvm.org/D53447

Modified:
    llvm/trunk/include/llvm/ADT/SparseBitVector.h
    llvm/trunk/unittests/ADT/SparseBitVectorTest.cpp

Modified: llvm/trunk/include/llvm/ADT/SparseBitVector.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SparseBitVector.h?rev=345772&r1=345771&r2=345772&view=diff
==============================================================================
--- llvm/trunk/include/llvm/ADT/SparseBitVector.h (original)
+++ llvm/trunk/include/llvm/ADT/SparseBitVector.h Wed Oct 31 13:05:32 2018
@@ -261,21 +261,33 @@ class SparseBitVector {
     BITWORD_SIZE = SparseBitVectorElement<ElementSize>::BITWORD_SIZE
   };
 
-  // Pointer to our current Element.
-  ElementListIter CurrElementIter;
+  // Pointer to our current Element. This has no visible effect on the external
+  // state of a SparseBitVector, it's just used to improve performance in the
+  // common case of testing/modifying bits with similar indices.
+  mutable ElementListIter CurrElementIter;
   ElementList Elements;
 
   // This is like std::lower_bound, except we do linear searching from the
   // current position.
-  ElementListIter FindLowerBound(unsigned ElementIndex) {
+  ElementListIter FindLowerBoundImpl(unsigned ElementIndex) const {
+
+    // We cache a non-const iterator so we're forced to resort to const_cast to
+    // get the begin/end in the case where 'this' is const. To avoid duplication
+    // of code with the only difference being whether the const cast is present
+    // 'this' is always const in this particular function and we sort out the
+    // difference in FindLowerBound and FindLowerBoundConst.
+    ElementListIter Begin =
+        const_cast<SparseBitVector<ElementSize> *>(this)->Elements.begin();
+    ElementListIter End =
+        const_cast<SparseBitVector<ElementSize> *>(this)->Elements.end();
 
     if (Elements.empty()) {
-      CurrElementIter = Elements.begin();
-      return Elements.begin();
+      CurrElementIter = Begin;
+      return CurrElementIter;
     }
 
     // Make sure our current iterator is valid.
-    if (CurrElementIter == Elements.end())
+    if (CurrElementIter == End)
       --CurrElementIter;
 
     // Search from our current iterator, either backwards or forwards,
@@ -284,17 +296,23 @@ class SparseBitVector {
     if (CurrElementIter->index() == ElementIndex) {
       return ElementIter;
     } else if (CurrElementIter->index() > ElementIndex) {
-      while (ElementIter != Elements.begin()
+      while (ElementIter != Begin
              && ElementIter->index() > ElementIndex)
         --ElementIter;
     } else {
-      while (ElementIter != Elements.end() &&
+      while (ElementIter != End &&
              ElementIter->index() < ElementIndex)
         ++ElementIter;
     }
     CurrElementIter = ElementIter;
     return ElementIter;
   }
+  ElementListConstIter FindLowerBoundConst(unsigned ElementIndex) const {
+    return FindLowerBoundImpl(ElementIndex);
+  }
+  ElementListIter FindLowerBound(unsigned ElementIndex) {
+    return FindLowerBoundImpl(ElementIndex);
+  }
 
   // Iterator to walk set bits in the bitmap.  This iterator is a lot uglier
   // than it would be, in order to be efficient.
@@ -464,12 +482,12 @@ public:
   }
 
   // Test, Reset, and Set a bit in the bitmap.
-  bool test(unsigned Idx) {
+  bool test(unsigned Idx) const {
     if (Elements.empty())
       return false;
 
     unsigned ElementIndex = Idx / ElementSize;
-    ElementListIter ElementIter = FindLowerBound(ElementIndex);
+    ElementListConstIter ElementIter = FindLowerBoundConst(ElementIndex);
 
     // If we can't find an element that is supposed to contain this bit, there
     // is nothing more to do.

Modified: llvm/trunk/unittests/ADT/SparseBitVectorTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/SparseBitVectorTest.cpp?rev=345772&r1=345771&r2=345772&view=diff
==============================================================================
--- llvm/trunk/unittests/ADT/SparseBitVectorTest.cpp (original)
+++ llvm/trunk/unittests/ADT/SparseBitVectorTest.cpp Wed Oct 31 13:05:32 2018
@@ -31,6 +31,11 @@ TEST(SparseBitVectorTest, TrivialOperati
   EXPECT_TRUE(Vec.test(17));
   Vec.clear();
   EXPECT_FALSE(Vec.test(17));
+
+  Vec.set(5);
+  const SparseBitVector<> ConstVec = Vec;
+  EXPECT_TRUE(ConstVec.test(5));
+  EXPECT_FALSE(ConstVec.test(17));
 }
 
 TEST(SparseBitVectorTest, IntersectWith) {