Reverse range adapter

[David Blaikie]

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?)

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"?

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?

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.



On Tue, Jul 28, 2015 at 3:35 PM, Pete Cooper <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> wrote:
>
>
> On Jul 28, 2015, at 9:59 AM, David Blaikie <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>
> 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
>> .
>>
>> 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
> 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/12f70de2/attachment.html>