Reverse range adapter

[David Blaikie]

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

>
> On Jul 29, 2015, at 2:44 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
> Drop the ", int" from enable_if and use nullptr instead of 0 for the
> default value (enable_if uses void* by default, so this is sort of more
> typical - of course the 0 works too)
>
> Perhaps add:
>
>   const TypeParam c = {0, 1, 2, 3};
>   TestRev(reverse(c));
>
> to TYPED_TEST(RangeAdapterLValueTest, TrivialOperation), to test the const
> cases?
>
> Done.  I remove the custom vector types from the lvalue test as they don’t
> support const_iterator’s.  But they are tested in the rvalue test so that
> should be fine.
>
>
> & run the whole thing through clang-format, if you haven't already? (I see
> some inconsistencies - like the space, or not, between the {} for the two
> Test derived class templates)
>
> Yeo, there were a few missing places.  I went over the changes made by
> clang format and all looked fine.
>
>
> & then feel free to commit at your leisure & we'll cross our fingers for
> the GCC bots.
>
> Great.  Thanks again for all the help.
>

Sure thing - 'preciate the patience.


>  I get the feeling as this point that you could have written it faster
> than helping me, so i really appreciate the guidance on this.
>

Perhaps, perhaps not - I probably would've missed a bunch of the things
I've caught in code review here. The benefit of being that second pair of
eyes. (forest, trees, etc)


>  Its r243581.
>

Awesome,
- Dave


>
> Pete
>
>
> On Wed, Jul 29, 2015 at 2:38 PM, Pete Cooper <peter_cooper at apple.com>
> wrote:
>
>>
>> On Jul 29, 2015, at 2:22 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>> On Wed, Jul 29, 2015 at 2:02 PM, Pete Cooper <peter_cooper at apple.com>
>> wrote:
>>
>>>
>>> On Jul 29, 2015, at 1:50 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>> 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})));
>>> }
>>>
>>> Very nice.  Thanks for doing that.
>>>
>>> Here’s a patch with the fixes.  I had to extend has_rbegin to handle
>>> whether its a class or not (int[] failed without that).  Otherwise its
>>> basically just like has_getDecl.  I think has_getDecl is in an internal
>>> namespace.  I can do the same here if you like, i’m fine either way.
>>>
>>
>> So the rvalue tests I added don't cover the non-rvalue case your existing
>> test cases covered (nor the array cases) - looks like you dropped the
>> originals?
>>
>> Sorry, i misread your email before.  I thought what you had replaced my
>> tests, now I see that it was the additional of rvalue tests.  I’ve
>> refactored what I had to use the TestRev method you have and added lvalue
>> tests.
>>
>>
>> I wonder if the SFINAE can be done without trying to derive from the type
>> (some types might not be derivable)
>>
>> Maybe the hasDecl was written pre-C++11?
>>
>> This /seems/ to work in the basic case:
>>
>> struct foo {
>>   void func();
>> };
>>
>> struct bar {
>> };
>>
>> template<typename T>
>> struct has_func {
>>   template<typename U>
>>   static char (&f(const U&, decltype(&U::func)))[1];
>>   static char (&f(...))[2];
>>   const static bool value = sizeof(f(std::declval<T>(), nullptr)) == 1;
>> };
>>
>> static_assert(has_func<foo>::value, "");
>> static_assert(!has_func<bar>::value, "”);
>>
>> Yeah, thats much better.  In fact there were a couple of cases failing
>> with my implementation when i brought back all the tests.  This version
>> handles everything.
>>
>>
>>
>>
>> (plus the extra non-class detection you had too)
>>
>>
>>>
>>> Cheers,
>>> Pete
>>>
>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150729/932d6bf7/attachment.html>