<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jul 22, 2015 at 12:11 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"><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>
</span>> <phinode.diff><br>
><br>
<br>
> +<br>
> +    const PHINodeEdge operator*() const {<br>
<br>
No reason for the const in `const PHINodeEdge` here.<br></blockquote><div><br></div><div>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*)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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 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>
</span>> 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.</blockquote><div><br>What's N1550 offer to make zip work?<br><br>I wouldn't mind a slightly half-hearted version that works for basic/common cases...  </div></div><br></div></div>