[PATCH] D33419: BitVector: use iterator_facade

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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:
> 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.
The usual way to do this (& other iterators have this sort of thing - consider, say, what the implementation of std::istream_iterator might look like) is to have a member of the value type fo the iterator, in the iterator - and, yes, it'd only be valid while the iterator hasn't been mutated (ie: you can take a reference to the element returned from iterator's op*, but you can't increment the iterator and continue to use the reference).

This, I think, is how most forward iterators are implemented. But I'm not 100% sure if it's necessary, just that it seems to be the norm in my experience.


https://reviews.llvm.org/D33419