<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jul 28, 2015 at 5:22 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"><br><div><span class=""><blockquote type="cite"><div>On Jul 28, 2015, at 5:13 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><div dir="ltr">I'd probably skip the pointer case and let callers dereference the pointer. I think that keeps the code a bit more obvious - avoids any weirdness/confusion around arrays of collections, etc (did this decay to a pointer then dereference that pointer and iterate the sub-collection, or what?)<br></div></div></blockquote></span>Good point.  Will remove it.<span class=""><br><blockquote type="cite"><div><div dir="ltr"><br>It might be easier to read the tests if the classes were interleaved with the test cases rather than "class A B C, test A B C”?<br></div></div></blockquote></span>I can do that, depending of course on how much of the simplification you mention merits even keeping the Vector classes at all.<span class=""><br><blockquote type="cite"><div><div dir="ltr"><br>The generalized case might be easier to read if we had a make_reverse_iterator to avoid the whole (decltype*2, std::end*2, std::begin*2)*2, etc?<br></div></div></blockquote></span>Good idea.  Will give that a try.<span class=""><br><blockquote type="cite"><div><div dir="ltr"><br>Could the test classes be made smaller/simpler? They don't need to be real collections - or if they are, perhaps we should just use real collections in those cases. (at least for the easy cases - eg: skip BidirectionalVector and just use std::vector directly, the other two probably at least don't need const/non-const overloads (doesn't seem like you're testing the const case and I'm not sure it would add much value to do so - but could consider it (maybe templated in some way to reduce duplication?)) - and perhaps just expose the vector rather than having push_back, given these are brief utilities (could have these containers constructed from the underlying container directly - so you populate that, then just create a wrapper)). gunit has a fancy test system that allows you to write one test as a template then run it with a set of types to instantiate the template with - that might apply here, but I'm not sure.<br></div></div></blockquote></span>I might need to keep the Bidirectional one just to ensure that we prefer rbegin() over reverse_iterator(begin()).  But otherwise i think you’re right about simplifying them.</div><div><br></div><div>I took a look at the standard library to see if any of the types there can only be iterated backwards. </div></div></blockquote><div><br>Yeah, I'd be surprised (if anywhere, I'd check std::forward_list - but I guess that only goes forwards, not backwards) - I would imagine anything that had only one iteration order would define that order to be forwards.<br><br>So yeah, a thin adapter that just has a member vector, perhaps, and rbegin/rend - or something similarly simple.<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> Thought perhaps queue or stack would only have rbegin() then i could use them instead of vector.  Unfortunately they are only protocols which use list and vector as their default implementations.<div><div class="h5"><br><blockquote type="cite"><div><div dir="ltr"><br><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jul 28, 2015 at 3:35 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">Once more update.  Seems I hadn’t handled pointers.  Added a variant which takes a pointer to a container and calls ->rbegin() and ->rend().<div><br></div><div></div></div><br><div style="word-wrap:break-word"><div><br><div><blockquote type="cite"><div>On Jul 28, 2015, at 10:19 AM, Pete Cooper <<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>> wrote:</div><br><div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><blockquote type="cite"><div><br>On Jul 28, 2015, at 9:59 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><div dir="ltr">I'm OK calling it 'reverse' as you have (since it has just the one argument it shouldn't be ambiguous with the iterator versions)<br><br>* These functions shouldn't be ‘static’</div></div></blockquote>Good point.  Made them inline like the other methods in the same file.<br><blockquote type="cite"><div><div dir="ltr">* Could you try using non-member begin/end in the second version - that should allow it to work with arrays. Give it a go/add a test?<br></div></div></blockquote>Done.  Added a test for this too.<br><blockquote type="cite"><div><div dir="ltr">* Maybe test the case where a container has rbegin/rend and begin/end to ensure we still favor the rbegin/rend (and that it's not ambiguous?) - presumably they're more efficient, if they're provided?<br></div></div></blockquote>Added a test for this too.  I left begin(), end() without method bodies so that if they were called we’d get linker errors.<br><blockquote type="cite"><div><div dir="ltr"><br>& the reason you don't need explicit SFINAE is because you put the interesting expressions in the return type - so they're part of the SFINAE condition already, conveniently.<br></div></div></blockquote>Makes sense.  Thanks for the explanation.<br><blockquote type="cite"><div><div dir="ltr"><br>I think Saleem (cc'd) had an existing implementation of something like this that he might be willing to provide some insight from?<br></div></div></blockquote>Cool.  Happy to see his implementation too, and to take whichever suits.</div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br></div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">Updated patch included.</div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br></div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">Cheers,</div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">Pete</div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br></div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"></div><span><reverse.patch></span><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br><blockquote type="cite"><div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jul 28, 2015 at 9:43 AM, Pete Cooper<span> </span><span dir="ltr"><<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@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"><div style="word-wrap:break-word">Hi David<div><br></div><div>Please find attached a patch for a reverse range adapter.  Its based on feedback you gave in <a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150720/289410.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150720/289410.html</a>.</div><div><br></div><div>There are 2 versions.  The first uses rbegin()/rend(), the second constructs std::reverse_iterators around begin()/end().</div><div><br></div><div>I was surprised to find I didn’t need enable_if or any other such tricks.</div><div><br></div><div>I’ve updated a single use of the pattern ‘for auto x : make_range(rbegin(), rend())’ to the new reverse method.</div><div><br></div><div>I was considering reverse_range instead as a name to avoid confusion with std::reverse.  I’d prefer to not do make_reverse_range just to save on characters.</div><div><br></div><div>Feedback welcome.</div><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></blockquote></div><br></div></div></blockquote></div><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing: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;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;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;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;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;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;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;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></blockquote></div></div></div><br></div></blockquote></div><br></div></div>