[PATCH] Add iterator for PHINode value/BB pair

[David Blaikie]

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


>
> >
> >
> > > +      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...
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150722/59b39759/attachment.html>