[PATCH] Add iterator for PHINode value/BB pair

[Duncan P. N. Exon Smith]

> On 2015-Jul-22, at 16:57, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Wed, Jul 22, 2015 at 4:50 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> > On 2015-Jul-22, at 15:07, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Wed, Jul 22, 2015 at 12:11 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> >
> > > On 2015-Jul-21, at 21:20, Pete Cooper <peter_cooper at apple.com> wrote:
> > >
> > > Thanks for all the feedback.  This is a patch which addresses all of it.
> > >
> > > <phinode.diff>
> > >
> >
> > > +
> > > +    const PHINodeEdge operator*() const {
> >
> > No reason for the const in `const PHINodeEdge` here.
> >
> > to support operator-> you have to return a pointer, which means you need the PHINodeEdge storage inside the iterator to point to (& then you can just return a const ref from op*)
> 
> Do we need operator->()?
> 
> Seems poor form not to provide it (someone'll trip over it pretty quickly, I'd imagine)
>  
> If so, we can return a proxy object:
> 
>     struct PHINodeEdgeArrowProxy {
>       const PHINodeEdge RefProxy;
>     public:
>       PHINodeEdgeArrowProxy(PHINodeEdge Edge) : RefProxy(Edge) {}
>       const PHINodeEdge *operator->() const { return &RefProxy; }
>     };
> 
>     PHINodeEdgeArrowProxy operator->() const { return operator*(); }
> 
> Then we avoid bloating the iterator, and only make the copy when we
> actually need it.
> 
> Non-conforming in terms of the iterator traits, I would imagine

AFAICT, iterators only require that `i->m` has the same semantics as
`(*i).m`.  The return type isn't specified.

> - but I take it that's the N1550 stuff you're talking about below? It makes these sort of proxy solutions valid?
>  

IIRC, proxy solutions are always valid for InputIterator and for
OutputIterator; it's ForwardIterator that prevents `operator*()`
from returning a proxy.

(This makes `std::vector<bool>::iterator` invalid.)

> 
> >
> >
> > > +      return { *Values, *Blocks };
> > > +    }
> > > +  };
> > > +
> >
> > Otherwise LGTM.  Might want to pass it through clang-format; I noticed
> > some minor whitespace oddities.
> >
> > > *snip*
> > >> On Jul 21, 2015, at 8:21 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> > >>
> > >>
> > >>
> > >> If you drop the requirements from `forward_iterator` to
> > >> `input_iterator`, then you're allowed to return a `PHINodeEdge` by-value
> > >> here instead of by-reference (unfortunately this makes it illegal to use
> > >> a bunch of STL algorithms; STL iterator traits are completely broken
> > >> IMO).
> > > I’m fine with this.  David, Chandler, please let me know how you feel about this.
> > >
> > > Also, i forgot to say that I considered doing a zip iterator and inheriting this from it.  This is something I think Chandler or David mentioned a few months ago.  If there’s been any progress in the C++ committee on that then i’m happy to try implement something better.  If not, then i don’t think what I have here should be difficult to change in future.
> >
> > I guess a generalized version would return a
> > `std::tuple<Value *const &, BasicBlock *const &>` or some such.  Not
> > sure how to actually make zip iterators work well without something
> > like N1550 though.
> >
> > What's N1550 offer to make zip work?
> 
> s/work/& well/
> 
> I think bloated iterators are bad, but without bloating them (so you
> can return a T&), you can't call a zip_iterator a ForwardIterator,
> which means you can't use it in a bunch of algorithms that you might
> want to (such as the destination for `std::copy()`).
> 
> N1550 let's you correctly identify the type of traversal the
> zip_iterator can do, without requiring a T& return from operator*().
> 
> > I wouldn't mind a slightly half-hearted version that works for basic/common cases...