Reverse range adapter
David Blaikie
dblaikie at gmail.com
Wed Jul 29 13:50:48 PDT 2015
On Wed, 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> wrote:
>
>
>
> 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.
>
> 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);
> }
>
Yep, something like that looks about right. I'm not sure if it can be
generalized further so we can reuse some of the machinery for begin, end,
rbegin, rend, getDecl, etc. You can probably just SFINAE on begin/rbegin
and assume end/rend exist - but if we can get the SFINAE tidy enough it
wouldn't hurt to test for both, I suppose.
btw, my change to the test that demonstrates the problem with using
universal ref/lvalue ref distinction to differentiate is this: add "const
ReverseOnlyVector/const BidirectionalVector" to the Types vector & you'll
see failures in the existing implementation. Alternatively/in addition,
could refactor out the body of the TrivialOperation test into a template,
then instantiate that template with TypeParam and const TypeParam, so you
don't have to list all of them as const/non-const in the list of types.
Also, I added this rvalue version:
template <typename T> struct RangeAdapterRValueTest : testing::Test {};
typedef ::testing::Types<std::vector<int>, std::list<int>,
ReverseOnlyVector,
BidirectionalVector> RangeAdapterRValueTestTypes;
TYPED_TEST_CASE(RangeAdapterRValueTest, RangeAdapterRValueTestTypes);
template<typename R>
void TestRev(const R& r) {
int counter = 3;
for (int i : r)
EXPECT_EQ(i, counter--);
}
TYPED_TEST(RangeAdapterRValueTest, TrivialOperation) {
TestRev(reverse(TypeParam({0, 1, 2, 3})));
}
>
>
>> 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/27ad6c5f/attachment.html>
More information about the llvm-commits
mailing list