Reverse range adapter

Wed Jul 29 13:04:46 PDT 2015

> On Jul 28, 2015, at 8:40 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> Oh, a couple of things - drop the 'inline' from the templates. That's just a hint to "inline harder" & usually isn't (shouldn't be) necessary. The implicit template specializations that come from the template will have the right linkonce_odr linkage & such that they'll be fine as-is.
Good point.  Done.
> 
> Also: probably not worth using auto/->decltype in make_reverse_iterator, I don't think? Looks like it'd be shorter to  just write the return type in the usual/old way?
Yeah, also done.
> 
> Oh, and you took the container by const ref in the rbegin/rend case, and non-const ref in the begin/end case - that seems strangely inconsistent?
Yeah, that was a mistake on my part.  Fixed.
> 
> I think /maybe/ you can take by rvalue ref and it'll do the right thing for const and non-const (would be good to test that - the current code should break for a const container that only has rbegin/rend, I think?)
So i’ve managed to add an rvalue reference to the begin/end case, but I couldn’t add it to both as it turns out the const was contributing to overload resolution.  My thinking for what we have now is that we are more likely to try call reverse on something like an iterator_range which only provides begin/end so it makes sense to put a preference on that one for the case where the source is a temporary rvalue.

If you aren’t happy with that choice, please let me know.

Anyway, committed with the LGTM as r243563.  Thanks for all the help on this.

Cheers,
Pete
> 
> On Tue, Jul 28, 2015 at 8:33 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
> LGTM, please commit.
> 
> One question for Richard Smith or someone else standards-y would be:
> 
> What's the right way to call non-member begin/end?
> 
> Should it be called unqualified, with a "using std::begin/end" like when we call std::swap? In which case, should we have some utility wrappers for this so it's easy to call in arbitrary contexts (llvm::adl_begin/end)?
> 
> On Tue, Jul 28, 2015 at 5:56 PM, Pete Cooper <peter_cooper at apple.com <mailto:peter_cooper at apple.com>> wrote:
> 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 <mailto: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/20150729/9403dd47/attachment.html>