[PATCH] D131448: Introduce iterator sentinel to make graph traversal implementation more efficient and cleaner

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

dexonsmith added inline comments.


================
Comment at: llvm/include/llvm/ADT/SCCIterator.h:165-170
+  SCCProxy operator*() const {
     assert(!CurrentSCC.empty() && "Dereferencing END SCC iterator!");
     return CurrentSCC;
   }
 
+  SCCProxy operator->() const { return operator*(); }
----------------
dblaikie wrote:
> rusyaev-roman wrote:
> > dblaikie wrote:
> > > I always forget in which cases you're allowed to return a proxy object from an iterator - I thought some iterator concepts (maybe random access is the level at which this kicks in?) that required something that amounts to "there has to be a real object that outlives the iterator"
> > > 
> > > Could you refresh my memory on that/on why proxy objects are acceptable for this iterator type? (where/how does this iterator declare what concept it models anyway, since this removed the facade helper?)
> > A proxy object is allowed to be returned while dereferencing an `input iterator` (https://en.cppreference.com/w/cpp/named_req/InputIterator#Notes)
> > 
> > ```
> > The reference type for an input iterator that is not also a LegacyForwardIterator does not have to be a reference type: dereferencing an input iterator may return a proxy object or value_type itself by value
> > ```
> > 
> > For our case (that's `forward iterator`) we need to satisfy the following thing:
> > ```
> >      The type std::iterator_traits<It>::reference must be exactly 
> >        ...
> >        * const T& otherwise (It is constant), 
> > 
> >     (where T is the type denoted by std::iterator_traits<It>::value_type) 
> > ```
> > I'll also update the patch according to this point. Other things are ok for using a proxy object.
> Thanks for doing the legwork/quotations there.
> 
> so what's the solution here, if we're going to meet the forward iterator requirements but want a proxy object?
IIRC, one solution is to store a proxy object inside the iterator, make `using value_type = ProxyT`, update what the stored proxy points at on `operator*()`, and return `ProxyT&` when dereferencing. But maybe I'm misremembering. (I'm pretty sure `iterator_facade_base` has machinery to help with this stuff, either way; might be worth looking at other uses of it.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131448