<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jul 23, 2015 at 12:29 PM, Pete Cooper <span dir="ltr"><<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Sorry this took so long.  Had to learn a whole bunch about varargs templates.<div><br></div><div>This is a first attempt at an LLVM zip iterator.  I needed to add methods to STLExtras.h which do ++tuple<> and *tuple<>.  Originally I put those under different names and didn’t override the operators, but then I didn’t see any harm in overriding them.  I can rename them if anyone has a preference for that.</div></div></blockquote><div><br></div><div>Yeah, those do make me twitch a bit - I'm not sure if there's a more coherent argument against them or not.</div><div><br></div><div>Oh, I see, this allows you to make the tuple itself an iterator. Yeah, also makes me twitch, but again - not sure if that's problematic or not. (I'm just imagining two libraries trying to pull the same game - and colliding in some painful way)</div><div><br></div><div>(regardless of the name, wonder if the ++ and * could be generalized over each other (presumably the latter, which is more powerful (since it has a return value) than the former - but I don't know if tuple<void, void, ...> is valid... ;)))</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>The zip iterator itself is almost identical to the edge_iterator I had before, just that it is now templated on its iterator type. </div></div></blockquote><div><br></div><div>There seem to be some naming issues here... wait, no, I see it - the tuple is the iterator (retcon'd comments in above once I realized this)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div> I’ve provided a subclass in zip_input_iterator for users who don’t need all the parameters and just want to provide a varargs of iterator types.  All of this is of course up for discussion, this is just one way I could see to implement it.<br></div></div></blockquote><div><br>Probably could keep it in one class rather than zip_iterator and zip_input_iterator - just always pass the collection of iterator types and then deduce the tag from the lowest common denominator of the iterator tags of the specified iterators (not sure if there's already a trait/etc for joining those different things together and getting the least powerful iterator tag as a result). Save us having a taxonomy of zip_*_iterators.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div></div><div>I also allowed for zip_iterator_traits so that users could provide their own type to return from operator*.  This is so that users don’t have to remember which iterator is first, second, get<0>, etc. </div></div></blockquote><div><br></div><div>Not sure if it's in 11 or 14, but there is type based access into tuple, get<Value*>, get<BasicBlock*>, etc - perhaps that's sufficient?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div> Personally I think its much more readable than to have .Value, .Block, etc.  This is optional though, as there’s a default implementation to provide a dereferenced tuple type from operator*.<br></div></div></blockquote><div><br>Interesting extension point, certainly. Could be problematic if there's the same pairing in two places with different meaning (int, int is size/length in one place, and something else elsewhere) and doesn't match up with different iterators (which might be weird to users - they change container type and then their code breaks because the trait is based on the iterator type, not the value type)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div></div><div>BTW, no idea how much of this is MSVC safe.  I did find a user of 'auto fn()->decltype()’ in PDBTypes.h, so i’m not the first user of that, but this will need someone with MSVC to ok the final implementation.<br></div><div><br></div><div>I also need to write a unit test for all the functionality this enables.  Duncan mentioned std::copy requiring operator* to return a reference.</div></div></blockquote><div><br></div><div>Interesting - I wonder how much/how it requires that? Because tuple<T&, U&> should be assignable (and assign to the reference members), etc, should work... even if the tuple is returned by value.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>  I’m still working through whether thats working or whether we only really have foreach support right now.</div><div><br></div><div>Duncan also pointed out in person, perhaps this is overkill as the edge_iterator I had before was easier to understand than what we have here.  Perhaps we’d prefer to hold off on a zip_iterator until we are sure we have more users for it.</div></div></blockquote><div><br></div><div>There are a fair few places where we iterate over two things in parallel and they're pretty awkward right now - could find a smattering of those and clean them up with the use of this tool (with a helper function to allow building these on-site within the range-for). I'm not sure of the easiest way to find places that could use this cleanup though... probably looking for for loops that either increment two things, or that use integers (because they're indexing into two containers).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>  If thats the case, then i’m happy for this to just be a demonstration of how it could be done, but not necessarily something to commit now.</div></div></blockquote><div><br></div><div>Fair, too.<br><br>- Dave</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><br></div><div>Cheers,</div><div>Pete</div><div><br></div><div></div></div><br><div style="word-wrap:break-word"><div></div><div><br></div><div><div><blockquote type="cite"><div>On Jul 22, 2015, at 5:10 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><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;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">(oops, dropped the list by accident)<br><div class="gmail_quote">---------- Forwarded message ----------<br>From:<span> </span><b class="gmail_sendername">David Blaikie</b><span> </span><span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span><br>Date: Wed, Jul 22, 2015 at 5:10 PM<br>Subject: Re: [PATCH] Add iterator for PHINode value/BB pair<br>To: "Duncan P. N. Exon Smith" <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>><br><br><br><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jul 22, 2015 at 5:05 PM, Duncan P. N. Exon Smith<span> </span><span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span><span> </span>wrote:<br><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>> On 2015-Jul-22, at 16:57, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>><br>><br>><br>> On Wed, Jul 22, 2015 at 4:50 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>><br>> > On 2015-Jul-22, at 15:07, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">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" target="_blank">dexonsmith@apple.com</a>> wrote:<span><br><span>> ><br>> > > On 2015-Jul-21, at 21:20, Pete Cooper <<a href="mailto:peter_cooper@apple.com" target="_blank">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>> Do we need operator->()?<br>><br>> Seems poor form not to provide it (someone'll trip over it pretty quickly, I'd imagine)<br>><br>> 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>><br>> Non-conforming in terms of the iterator traits, I would imagine<br><br></span>AFAICT, iterators only require that `i->m` has the same semantics as<br>`(*i).m`.  The return type isn't specified.<br></span></blockquote><div><br></div><div>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><div> </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>> - but I take it that's the N1550 stuff you're talking about below? It makes these sort of proxy solutions valid?<br>><br><br></span>IIRC, proxy solutions are always valid for InputIterator and for<br>OutputIterator; it's ForwardIterator that prevents `operator*()`<br>from returning a proxy.<br></blockquote></span><div><br>*nod* I don't mind violating that too much, if that's the preference.<br> </div><span><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></blockquote><div><br></div></span><div>Yep. Fails in so many ways.</div><div> </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><br>><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><div><div>> > >> On Jul 21, 2015, at 8:21 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">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>> 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>><br>> > I wouldn't mind a slightly half-hearted version that works for basic/common cases...<br><br></div></div></span></blockquote></div><br></div></div></div><br></div><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">_______________________________________________</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">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;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><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;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" target="_blank">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;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><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;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></div></blockquote></div><br></div></div><br></blockquote></div><br></div></div>