Reverse range adapter

[David Blaikie]

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?

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

& then feel free to commit at your leisure & we'll cross our fingers for
the GCC bots.

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/c9635b67/attachment.html>