<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""></div><div class=""><br class=""></div><div class=""><div><blockquote type="cite" class=""><div class="">On Jul 22, 2015, at 5:10 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">(oops, dropped the list by accident)<br class=""><div class="gmail_quote">---------- Forwarded message ----------<br class="">From:<span class="Apple-converted-space"> </span><b class="gmail_sendername">David Blaikie</b><span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>></span><br class="">Date: Wed, Jul 22, 2015 at 5:10 PM<br class="">Subject: Re: [PATCH] Add iterator for PHINode value/BB pair<br class="">To: "Duncan P. N. Exon Smith" <<a href="mailto:dexonsmith@apple.com" class="">dexonsmith@apple.com</a>><br class=""><br class=""><br class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Wed, Jul 22, 2015 at 5:05 PM, Duncan P. N. Exon Smith<span class="Apple-converted-space"> </span><span dir="ltr" class=""><<a href="mailto:dexonsmith@apple.com" target="_blank" class="">dexonsmith@apple.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><br class="">> On 2015-Jul-22, at 16:57, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:<br class="">><br class="">><br class="">><br class="">> On Wed, Jul 22, 2015 at 4:50 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank" class="">dexonsmith@apple.com</a>> wrote:<br class="">><br class="">> > On 2015-Jul-22, at 15:07, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:<br class="">> ><br class="">> ><br class="">> ><br class="">> > On Wed, Jul 22, 2015 at 12:11 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank" class="">dexonsmith@apple.com</a>> wrote:<span class=""><br class=""><span class="">> ><br class="">> > > On 2015-Jul-21, at 21:20, Pete Cooper <<a href="mailto:peter_cooper@apple.com" target="_blank" class="">peter_cooper@apple.com</a>> wrote:<br class="">> > ><br class="">> > > Thanks for all the feedback. This is a patch which addresses all of it.<br class="">> > ><br class="">> > > <phinode.diff><br class="">> > ><br class="">> ><br class="">> > > +<br class="">> > > + const PHINodeEdge operator*() const {<br class="">> ><br class="">> > No reason for the const in `const PHINodeEdge` here.<br class="">> ><br class="">> > 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 class="">><br class="">> Do we need operator->()?<br class="">><br class="">> Seems poor form not to provide it (someone'll trip over it pretty quickly, I'd imagine)<br class="">><br class="">> If so, we can return a proxy object:<br class="">><br class="">> struct PHINodeEdgeArrowProxy {<br class="">> const PHINodeEdge RefProxy;<br class="">> public:<br class="">> PHINodeEdgeArrowProxy(PHINodeEdge Edge) : RefProxy(Edge) {}<br class="">> const PHINodeEdge *operator->() const { return &RefProxy; }<br class="">> };<br class="">><br class="">> PHINodeEdgeArrowProxy operator->() const { return operator*(); }<br class="">><br class="">> Then we avoid bloating the iterator, and only make the copy when we<br class="">> actually need it.<br class="">><br class="">> Non-conforming in terms of the iterator traits, I would imagine<br class=""><br class=""></span>AFAICT, iterators only require that `i->m` has the same semantics as<br class="">`(*i).m`. The return type isn't specified.<br class=""></span></blockquote><div class=""><br class=""></div><div class="">Fair enough - can't quite find the wording on what constitutes the iterators value type (as is mentioned in the iterator traits) or how it might relate, but the basic definition is as you've mentioned.</div><span class=""><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><span class="">> - but I take it that's the N1550 stuff you're talking about below? It makes these sort of proxy solutions valid?<br class="">><br class=""><br class=""></span>IIRC, proxy solutions are always valid for InputIterator and for<br class="">OutputIterator; it's ForwardIterator that prevents `operator*()`<br class="">from returning a proxy.<br class=""></blockquote></span><div class=""><br class="">*nod* I don't mind violating that too much, if that's the preference.<br class=""> </div><span class=""><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;">(This makes `std::vector<bool>::iterator` invalid.)<br class=""></blockquote><div class=""><br class=""></div></span><div class="">Yep. Fails in so many ways.</div><div class=""> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><span class=""><br class="">><br class="">> ><br class="">> ><br class="">> > > + return { *Values, *Blocks };<br class="">> > > + }<br class="">> > > + };<br class="">> > > +<br class="">> ><br class="">> > Otherwise LGTM. Might want to pass it through clang-format; I noticed<br class="">> > some minor whitespace oddities.<br class="">> ><br class="">> > > *snip*<br class=""></span><span class=""><div class=""><div class="">> > >> On Jul 21, 2015, at 8:21 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank" class="">dexonsmith@apple.com</a>> wrote:<br class="">> > >><br class="">> > >><br class="">> > >><br class="">> > >> If you drop the requirements from `forward_iterator` to<br class="">> > >> `input_iterator`, then you're allowed to return a `PHINodeEdge` by-value<br class="">> > >> here instead of by-reference (unfortunately this makes it illegal to use<br class="">> > >> a bunch of STL algorithms; STL iterator traits are completely broken<br class="">> > >> IMO).<br class="">> > > I’m fine with this. David, Chandler, please let me know how you feel about this.<br class="">> > ><br class="">> > > 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 class="">> ><br class="">> > I guess a generalized version would return a<br class="">> > `std::tuple<Value *const &, BasicBlock *const &>` or some such. Not<br class="">> > sure how to actually make zip iterators work well without something<br class="">> > like N1550 though.<br class="">> ><br class="">> > What's N1550 offer to make zip work?<br class="">><br class="">> s/work/& well/<br class="">><br class="">> I think bloated iterators are bad, but without bloating them (so you<br class="">> can return a T&), you can't call a zip_iterator a ForwardIterator,<br class="">> which means you can't use it in a bunch of algorithms that you might<br class="">> want to (such as the destination for `std::copy()`).<br class="">><br class="">> N1550 let's you correctly identify the type of traversal the<br class="">> zip_iterator can do, without requiring a T& return from operator*().<br class="">><br class="">> > I wouldn't mind a slightly half-hearted version that works for basic/common cases...<br class=""><br class=""></div></div></span></blockquote></div><br class=""></div></div></div><br class=""></div><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">_______________________________________________</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">llvm-commits mailing list</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="mailto:llvm-commits@cs.uiuc.edu" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">llvm-commits@cs.uiuc.edu</a><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></div></blockquote></div><br class=""></div></body></html>