<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jul 22, 2015 at 4:50 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> On 2015-Jul-22, at 15:07, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Wed, Jul 22, 2015 at 12:11 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
<span class="">><br>
> > On 2015-Jul-21, at 21:20, Pete Cooper <<a href="mailto:peter_cooper@apple.com">peter_cooper@apple.com</a>> wrote:<br>
> ><br>
> > Thanks for all the feedback.  This is a patch which addresses all of it.<br>
> ><br>
> > <phinode.diff><br>
> ><br>
><br>
> > +<br>
> > +    const PHINodeEdge operator*() const {<br>
><br>
> No reason for the const in `const PHINodeEdge` here.<br>
><br>
> 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*)<br>
<br>
</span>Do we need operator->()?<br></blockquote><div><br></div><div>Seems poor form not to provide it (someone'll trip over it pretty quickly, I'd imagine)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">If so, we can return a proxy object:<br>
<br>
    struct PHINodeEdgeArrowProxy {<br>
      const PHINodeEdge RefProxy;<br>
    public:<br>
      PHINodeEdgeArrowProxy(PHINodeEdge Edge) : RefProxy(Edge) {}<br>
      const PHINodeEdge *operator->() const { return &RefProxy; }<br>
    };<br>
<br>
    PHINodeEdgeArrowProxy operator->() const { return operator*(); }<br>
<br>
Then we avoid bloating the iterator, and only make the copy when we<br>
actually need it.<br></blockquote><div><br>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?<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
><br>
><br>
> > +      return { *Values, *Blocks };<br>
> > +    }<br>
> > +  };<br>
> > +<br>
><br>
> Otherwise LGTM.  Might want to pass it through clang-format; I noticed<br>
> some minor whitespace oddities.<br>
><br>
> > *snip*<br>
</span><span class="">> >> On Jul 21, 2015, at 8:21 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> >><br>
> >><br>
> >><br>
> >> If you drop the requirements from `forward_iterator` to<br>
> >> `input_iterator`, then you're allowed to return a `PHINodeEdge` by-value<br>
> >> here instead of by-reference (unfortunately this makes it illegal to use<br>
> >> a bunch of STL algorithms; STL iterator traits are completely broken<br>
> >> IMO).<br>
> > I’m fine with this.  David, Chandler, please let me know how you feel about this.<br>
> ><br>
> > 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.<br>
><br>
> I guess a generalized version would return a<br>
> `std::tuple<Value *const &, BasicBlock *const &>` or some such.  Not<br>
> sure how to actually make zip iterators work well without something<br>
> like N1550 though.<br>
><br>
> What's N1550 offer to make zip work?<br>
<br>
</span>s/work/& well/<br>
<br>
I think bloated iterators are bad, but without bloating them (so you<br>
can return a T&), you can't call a zip_iterator a ForwardIterator,<br>
which means you can't use it in a bunch of algorithms that you might<br>
want to (such as the destination for `std::copy()`).<br>
<br>
N1550 let's you correctly identify the type of traversal the<br>
zip_iterator can do, without requiring a T& return from operator*().<br>
<div class="HOEnZb"><div class="h5"><br>
> I wouldn't mind a slightly half-hearted version that works for basic/common cases...<br>
<br>
</div></div></blockquote></div><br></div></div>