Reverse range adapter
Pete Cooper
peter_cooper at apple.com
Wed Jul 29 13:30:44 PDT 2015
Seems the GCC bots don’t like this at all. They fail in overload resolution even though clang accepts what I did. The log is http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15/builds/4582/steps/build%20stage%201/logs/stdio.
I’ve reverted in r243567 until we can do something with enable_if.
Pete
> On Jul 29, 2015, at 1:22 PM, Pete Cooper <peter_cooper at apple.com> wrote:
>
>>
>> On Jul 29, 2015, at 1:16 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>>
>>
>>
>> On Wed, Jul 29, 2015 at 1:04 PM, Pete Cooper <peter_cooper at apple.com <mailto:peter_cooper at apple.com>> wrote:
>>
>>> On Jul 28, 2015, at 8:40 PM, David Blaikie <dblaikie at gmail.com <mailto: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.
> Thanks!
>
> So i found this in clang. I think we just need something similar, with appropriate enable_if on existence or not. What do you think?
>
> /// \brief Metafunction to determine if type T has a member called getDecl.
> template <typename T> struct has_getDecl {
> struct Default { int getDecl; };
> struct Derived : T, Default { };
>
> template<typename C, C> struct CheckT;
>
> // If T::getDecl exists, an ambiguity arises and CheckT will
> // not be instantiable. This makes f(...) the only available
> // overload.
> template<typename C>
> static char (&f(CheckT<int Default::*, &C::getDecl>*))[1];
> template<typename C> static char (&f(...))[2];
>
> static bool const value = sizeof(f<Derived>(nullptr)) == 2;
> };
>
> template <typename U>
> bool matchesSpecialized(
> const U &Node, ASTMatchFinder *Finder, BoundNodesTreeBuilder *Builder,
> typename std::enable_if<has_getDecl<U>::value, int>::type = 0) const {
> return matchesDecl(Node.getDecl(), Finder, Builder);
> }
>>
>> 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>
> _______________________________________________
> 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/cc3282ac/attachment.html>
More information about the llvm-commits
mailing list