[PATCH] D33419: BitVector: use iterator_facade

[David Blaikie via Phabricator via llvm-commits]

dblaikie requested changes to this revision.
dblaikie added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/ADT/iterator.h:45-49
+///   - either
+///            - const T &operator*() const;
+///            - T &operator*();
+///   - or
+///            - T operator*() const;
----------------
Actually, I see your leading comment and this change now.

I'm not sure if this change is valid/necessary. I /think/ op* is meant to return something convertible to the reference type of the iterator, even for a forward iterator. (& you should be able to take its address and use that so long as you don't modify the iterator).

ie: I /think/ this should be valid for all forward iterators:

  auto &x = *my_iterator;
  func(x);

but for the bit iterator you've added, that would be invalid - reference to temporary, etc.

I /think/ you have to return a (const, in this case) unsigned& from the op* and you should be fine & not need these changes)


https://reviews.llvm.org/D33419