Reverse range adapter

Wed Jul 29 13:16:29 PDT 2015

On Wed, Jul 29, 2015 at 1:04 PM, Pete Cooper <peter_cooper at apple.com> wrote:

>
> 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.
>

This is probably where we need explicit SFINAE to make the begin/end
version go away when the rbegin/rend version will kick in. I'll check the
test cases & see what's missing/avoided/not working because of this absence.

>  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> 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>
>> 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> wrote:
>>>
>>>
>>>
>>> On Tue, Jul 28, 2015 at 5:22 PM, Pete Cooper <peter_cooper at apple.com>
>>> wrote:
>>>
>>>>
>>>> On Jul 28, 2015, at 5:13 PM, David Blaikie <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>
>>>> 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/20150729/6092ecc8/attachment.html>