[PATCH] Add iterator for PHINode value/BB pair

[Duncan P. N. Exon Smith]

> 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->()?

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.

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