[PATCH] D32060: BitVector: add iterators for bits that are set

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

Thanks for working on this. I missed this feature myself a couple of times.

We generally do not commit unused code, so before commit this we have to wait until one of the shrink-wrapping patches is accepted. Or you go around llvm and try to find a few `find_next()` users that can be replaced with a range based for.



================
Comment at: include/llvm/ADT/BitVector.h:79-80
+  /// Iterators get invalidated when resize / reserve is called.
+  template <typename BitVectorT>
+  class const_set_iterator_impl {
+    const BitVectorT *Parent = nullptr;
----------------
You should probably pull this out of the class, as it doesn't use any private methods and is used not just by BitVector but also by SmallBitVector.


================
Comment at: include/llvm/ADT/BitVector.h:81
+  class const_set_iterator_impl {
+    const BitVectorT *Parent = nullptr;
+    int Current = 0;
----------------
Does this ever make sense to be nullptr? I'd recommend using a reference.


================
Comment at: include/llvm/ADT/BitVector.h:87
+        : Parent(Parent), Current(Current) {}
+    const_set_iterator_impl(const BitVectorT *Parent)
+        : const_set_iterator_impl(Parent, Parent->find_first()) {}
----------------
I'd recommend an `explicit` modifier here.


================
Comment at: include/llvm/ADT/BitVector.h:91-94
+    void advance() {
+      assert(Current != -1 && "Trying to advance past end.");
+      Current = Parent->find_next(Current);
+    }
----------------
This can be private.


================
Comment at: include/llvm/ADT/BitVector.h:107
+
+    int operator*() const { return Current; }
+    const BitVectorT *getParent() const { return Parent; }
----------------
We have `size_type BitVector::size()` or `BitVector::set(unsigned Idx)`. We seem to be a bit inconsistent regarding size_type vs. unsigned but the majority of functions appear to be using `unsigned` for bit indexes so this should be the return type here.



================
Comment at: include/llvm/ADT/BitVector.h:108
+    int operator*() const { return Current; }
+    const BitVectorT *getParent() const { return Parent; }
+  };
----------------
Is it necessary to make this accessible? Having access to the parent bitvector seems like an implementation detail to me that could stay private.


================
Comment at: include/llvm/ADT/BitVector.h:111-118
+  typedef const_set_iterator_impl<BitVector> const_set_iterator;
+  typedef const_set_iterator set_iterator;
+
+  const_set_iterator set_begin() const { return {this}; }
+  const_set_iterator set_end() const { return {this, -1}; }
+  iterator_range<const_set_iterator> bits_set() const {
+    return make_range(set_begin(), set_end());
----------------
This could be more consistent: `const_bits_set_iterator`, `bit_set_begin()` and `bit_set()`.


================
Comment at: include/llvm/ADT/BitVector.h:114-115
+
+  const_set_iterator set_begin() const { return {this}; }
+  const_set_iterator set_end() const { return {this, -1}; }
+  iterator_range<const_set_iterator> bits_set() const {
----------------
http://llvm.org/docs/CodingStandards.html#do-not-use-braced-initializer-lists-to-call-a-constructor recommends against calling constructors with curly braces.


================
Comment at: include/llvm/ADT/BitVector.h:665-679
+template <typename BitVectorT>
+bool operator==(const BitVector::const_set_iterator_impl<BitVectorT> &This,
+                const BitVector::const_set_iterator_impl<BitVectorT> &Other) {
+  assert(This.getParent() == Other.getParent() &&
+         "Comparing iterators from different BitVectors");
+  return *This == *Other;
+}
----------------
Isn't it possible to move those into the iterator class?


================
Comment at: unittests/ADT/BitVectorTest.cpp:514
+  for (int Bit : Empty.bits_set())
+    EXPECT_FALSE(Bit != 9999);
+
----------------
How about `EXPECT_TRUE(false);` to make it more obvious?


================
Comment at: unittests/ADT/BitVectorTest.cpp:526-533
+  size_t Count = ToFill.count();
+  EXPECT_EQ((int)Count, 5);
+  size_t ItCount = 0;
+  for (int Bit : ToFill.bits_set()) {
+    (void)Bit;
+    ++ItCount;
+  }
----------------
Maybe construct an array {1,10,99,25} that you verify against during the iteration to make sure we do indeed return the right numbers?


https://reviews.llvm.org/D32060