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

[Roman Rusyaev via Phabricator via llvm-commits]

rusyaev-roman 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*(); }
----------------
dexonsmith wrote:
> 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.)
> so what's the solution here, if we're going to meet the forward iterator requirements but want a proxy object?

We need to modify the code according to the above comment. I'm highlighting it below.  (maybe, I was not clear. Sorry for this. @dexonsmith has provided more detailed explanation)


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131448