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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
