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

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

In D117116#3240612 <https://reviews.llvm.org/D117116#3240612>, @jansvoboda11 wrote:

> 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.



>   for (auto Bit : Bits) Bit = true; // modifies Bits through BitVector::reference

Agreed this would be unexpected.

>   for (bool &Bit : Bits) ; // fails to compile, cannot bind non-const lvalue reference to temporary BitVector::reference

Not really worried about this. The compiler error teaches you to spell it `for (auto &Bit : Bits)`. And I think the benefit of `BitVector::iterator` would be more for generic algorithms (like `zip_first()`) than for a ranged-based `for` anyway.

I don't feel strongly that `iterator` should be added... but I also think using BitVector (or a `std::vector<bool>`) in a range-based `for` at all is a bit of an odd pattern, so the primary benefit I see of adding iterators at all is enabling parallel iteration/etc (generic algorithms). As long as those work I'm probably fine with these gotchas.

What happens for something like `zip_first()`/etc. from STLExtras.h?



================
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;
----------------
jansvoboda11 wrote:
> 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?
Agreed on the `iterator_facade_base` documentation bug; it should probably say `ReferenceT`.

> The iterator at hand should be random access iterator though. Do we care about the standard here?

I think the workaround for this is for the iterator to store a `bool` that gets updated on `operator++()`; then you can return `const bool &`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117116