[PATCH] D33419: BitVector: use iterator_facade

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg added inline comments.


================
Comment at: include/llvm/ADT/iterator.h:45-49
+///   - either
+///            - const T &operator*() const;
+///            - T &operator*();
+///   - or
+///            - T operator*() const;
----------------
thegameg wrote:
> dblaikie wrote:
> > 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)
> I'm not sure I understand how that would work, since `find_first` / `find_next` return an int, but we actually want the `operator*` to return an unsigned. We could return a `const int&`, but as @MatzeB pointed out [[ https://reviews.llvm.org/D32060?id=95232#inline-277829 | here ]], it makes much more sense to return an unsigned.
And this gets even more dangerous since the reference would be bound to the lifetime of the iterator, not the container.


https://reviews.llvm.org/D33419