Reverse range adapter

Tue Jul 28 17:56:57 PDT 2015

New patch attached.

I added the make_reverse_iterator method which definitely simplifies that version of reverse.

The unit test has also been updated to use the template mechanism (very cool btw!).  I had to keep 2 of the vector types i’d defined (to restrict whether they had begin() or begin() or both), but removed the const iterators and push_back.  They have a constructor from std::initializer_list so that I can share the test case with all the container types.

Pete

> On Jul 28, 2015, at 5:27 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Tue, Jul 28, 2015 at 5:22 PM, Pete Cooper <peter_cooper at apple.com <mailto:peter_cooper at apple.com>> wrote:
> 
>> On Jul 28, 2015, at 5:13 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>> 
>> 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?)
> Good point.  Will remove it.
>> 
>> 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”?
> I can do that, depending of course on how much of the simplification you mention merits even keeping the Vector classes at all.
>> 
>> 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?
> Good idea.  Will give that a try.
>> 
>> 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.
> 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.
> 
> I took a look at the standard library to see if any of the types there can only be iterated backwards.
> 
> 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.
> 
> So yeah, a thin adapter that just has a member vector, perhaps, and rbegin/rend - or something similarly simple.
>  
>  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.
> 
>> 
>> 
>> 
>> On Tue, Jul 28, 2015 at 3:35 PM, Pete Cooper <peter_cooper at apple.com <mailto:peter_cooper at apple.com>> wrote:
>> Once more update.  Seems I hadn’t handled pointers.  Added a variant which takes a pointer to a container and calls ->rbegin() and ->rend().
>> 
>> 
>> 
>>> On Jul 28, 2015, at 10:19 AM, Pete Cooper <peter_cooper at apple.com <mailto:peter_cooper at apple.com>> wrote:
>>> 
>>>> 
>>>> On Jul 28, 2015, at 9:59 AM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>>>> 
>>>> 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)
>>>> 
>>>> * These functions shouldn't be ‘static’
>>> Good point.  Made them inline like the other methods in the same file.
>>>> * 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?
>>> Done.  Added a test for this too.
>>>> * 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?
>>> Added a test for this too.  I left begin(), end() without method bodies so that if they were called we’d get linker errors.
>>>> 
>>>> & 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.
>>> Makes sense.  Thanks for the explanation.
>>>> 
>>>> I think Saleem (cc'd) had an existing implementation of something like this that he might be willing to provide some insight from?
>>> Cool.  Happy to see his implementation too, and to take whichever suits.
>>> 
>>> Updated patch included.
>>> 
>>> Cheers,
>>> Pete
>>> 
>>> <reverse.patch>
>>> 
>>>> 
>>>> On Tue, Jul 28, 2015 at 9:43 AM, Pete Cooper <peter_cooper at apple.com <mailto:peter_cooper at apple.com>> wrote:
>>>> Hi David
>>>> 
>>>> Please find attached a patch for a reverse range adapter.  Its based on feedback you gave in http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150720/289410.html <http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150720/289410.html>.
>>>> 
>>>> There are 2 versions.  The first uses rbegin()/rend(), the second constructs std::reverse_iterators around begin()/end().
>>>> 
>>>> I was surprised to find I didn’t need enable_if or any other such tricks.
>>>> 
>>>> I’ve updated a single use of the pattern ‘for auto x : make_range(rbegin(), rend())’ to the new reverse method.
>>>> 
>>>> 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.
>>>> 
>>>> Feedback welcome.
>>>> 
>>>> Cheers,
>>>> Pete
>>>> 
>>>> 
>>>> 
>>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
>> 
>> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150728/efc5ab0b/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: reverse.patch
Type: application/octet-stream
Size: 5158 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150728/efc5ab0b/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150728/efc5ab0b/attachment-0001.html>