[PATCH] D117116: [llvm][ADT] Enable range-based for loops for `BitVector`

[Jan Svoboda via Phabricator via llvm-commits]

jansvoboda11 marked an inline comment as done.
jansvoboda11 added a comment.

In D117116#3238579 <https://reviews.llvm.org/D117116#3238579>, @dexonsmith wrote:

> Given that `BitVector::reference` already exists for `operator[]`, might be nice to add `iterator` as well.

That sounds nice, but there are two pitfalls exposed by non-const iterators.

The first:

  void test(BitVector &Bits) {
    for (bool Bit : Bits) Bit = true; // no effect, we have a copy
    for (auto Bit : Bits) Bit = true; // modifies Bits through BitVector::reference
  }

The second:

  void test(const BitVector &Bits) {
    for (const bool &Bit : Bits) ; // works fine
  }
  void test(      BitVector &Bits) {
    for (      bool &Bit : Bits) ; // fails to compile, cannot bind non-const lvalue reference to temporary BitVector::reference
  }

Is that worth the trouble? Do we have ways to deal with this?



================
Comment at: llvm/include/llvm/ADT/BitVector.h:33
+/// Iterators get invalidated when resize / reserve is called.
+template <typename BitVectorT> class const_all_bits_iterator_impl {
+  using size_type = typename BitVectorT::size_type;
----------------
dexonsmith wrote:
> It'd probably be easiest to use `iterator_facade_base` (from `ADT/Iterator.h` IIRC?). Value type should be `const bool` here.
Thanks for the pointer to `iterator_facade_base`.

Its documentation states my iterator should implement `const T &operator*() const`. However, contents of `BitVector` are being handed out by value (`bool operator[](unsigned) const`). Naively returning that from iterator's `operator*` would return reference to a temporary. I don't see a simple solution/workaround to this with the current `iterator_facade_base` requirements.

But I think there's a bug in `iterator_facade_base`'s documentation: it should probably say the iterator should implement `ReferenceT operator*() const` (where `ReferenceT = T &`) by default.

If we fix this, we could create new `BitVector::const_reference` type, make it the return type of `BitVector::operator[](unsigned) const` and explicitly specify it as the `ReferenceT` for `iterator_facade_base`. I think there is precedent in LLVM for this.

But looking at the standard, all iterator types except input iterators require the `reference` type to be a reference type (`T &`). The iterator at hand should be random access iterator though. Do we care about the standard here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117116/new/

https://reviews.llvm.org/D117116