From f777e8b463da352e63679bfa7968cbec7468f1f9 Mon Sep 17 00:00:00 2001
From: Daniel Sanders <daniel_l_sanders@apple.com>
Date: Wed, 31 Oct 2018 20:05:32 +0000
Subject: [PATCH] [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

llvm-svn: 345772
---
 llvm/include/llvm/ADT/SparseBitVector.h    | 38 ++++++++++++++++------
 llvm/unittests/ADT/SparseBitVectorTest.cpp |  5 +++
 2 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/llvm/include/llvm/ADT/SparseBitVector.h b/llvm/include/llvm/ADT/SparseBitVector.h
index 4cbf40c76805..09a91b6614ed 100644
--- a/llvm/include/llvm/ADT/SparseBitVector.h
+++ b/llvm/include/llvm/ADT/SparseBitVector.h
@@ -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.
diff --git a/llvm/unittests/ADT/SparseBitVectorTest.cpp b/llvm/unittests/ADT/SparseBitVectorTest.cpp
index 9d6f4f1665d5..097f4a0b737e 100644
--- a/llvm/unittests/ADT/SparseBitVectorTest.cpp
+++ b/llvm/unittests/ADT/SparseBitVectorTest.cpp
@@ -31,6 +31,11 @@ TEST(SparseBitVectorTest, TrivialOperation) {
   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) {