Reverse range adapter

David Blaikie dblaikie at gmail.com
Wed Jul 29 17:06:13 PDT 2015 

On Wed, Jul 29, 2015 at 5:02 PM, Filipe Cabecinhas <filcab at gmail.com> wrote:

> Hi Pete, David,
>
> This is still failing for -std=c++1y (latest released Mac OS + Xcode. call
> cmake with -DLLVM_USE_CXX1Y=YES):
>
> [1117/25/457] Building CXX object
> lib/CodeGen/AsmPrinter/CMakeFiles/LLVMAsmPrinter.dir/ARMException.cpp.o
> FAILED:
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++
>   -DGTEST_HAS_RTTI=0 -D_DEBUG -D__STDC_CONSTANT_MACROS
> -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -stdlib=libc++ -fPIC
> -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings
> -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long
> -Wcovered-switch-default -Wnon-virtual-dtor -std=c++1y -fcolor-diagnostics
> -O3  -Ilib/CodeGen/AsmPrinter
> -I/Users/filcab/work/llvm/lib/CodeGen/AsmPrinter -Iinclude
> -I/Users/filcab/work/llvm/include    -UNDEBUG -fno-exceptions -fno-rtti
> -MMD -MT
> lib/CodeGen/AsmPrinter/CMakeFiles/LLVMAsmPrinter.dir/ARMException.cpp.o -MF
> lib/CodeGen/AsmPrinter/CMakeFiles/LLVMAsmPrinter.dir/ARMException.cpp.o.d
> -o lib/CodeGen/AsmPrinter/CMakeFiles/LLVMAsmPrinter.dir/ARMException.cpp.o
> -c /Users/filcab/work/llvm/lib/CodeGen/AsmPrinter/ARMException.cpp
> /Users/filcab/work/llvm/lib/CodeGen/AsmPrinter/ARMException.cpp:118:32:
> error: no matching function for call to 'reverse'
>   for (const GlobalValue *GV : reverse(TypeInfos)) {
>                                ^~~~~~~
> /Users/filcab/work/llvm/include/llvm/ADT/STLExtras.h:210:38: note:
> candidate template ignored: disabled by 'enable_if' [with ContainerTy =
> const std::__1::vector<const llvm::GlobalValue *, std::__1::allocator<const
> llvm::GlobalValue *> > &]
>              typename std::enable_if<has_rbegin<ContainerTy>::value>::type
> * =
>                                      ^
> /Users/filcab/work/llvm/include/llvm/ADT/STLExtras.h:225:6: note:
> candidate template ignored: substitution failure [with ContainerTy = const
> std::__1::vector<const llvm::GlobalValue *, std::__1::allocator<const
> llvm::GlobalValue *> > &]: call to 'make_reverse_iterator' is ambiguous
>

Is there a std::make_reverse_iterator in 1y? Hey, there totally is one in
14: http://en.cppreference.com/w/cpp/iterator/make_reverse_iterator

Pete - you should probably be able to fix this by just qualifying the call
with llvm::make_reverse_iterator

(same way we do with make_unique to workaround MSVC providing
std::make_unique)


> auto reverse(
>      ^
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/algorithm:2335:1:
> note: candidate function template not viable: requires 2 arguments, but 1
> was provided
> reverse(_BidirectionalIterator __first, _BidirectionalIterator __last)
> ^
> 1 error generated.
> [1117/0/482] Building CXX object
> lib/IR/CMakeFiles/LLVMCore.dir/Function.cpp.o
> ninja: build stopped: subcommand failed.
>
> C++11 works:
> [llvm-cmake]%
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++
>   -DGTEST_HAS_RTTI=0 -D_DEBUG -D__STDC_CONSTANT_MACROS
> -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -stdlib=libc++ -fPIC
> -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings
> -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long
> -Wcovered-switch-default -Wnon-virtual-dtor -std=c++11 -fcolor-diagnostics
> -O3  -Ilib/CodeGen/AsmPrinter
> -I/Users/filcab/work/llvm/lib/CodeGen/AsmPrinter -Iinclude
> -I/Users/filcab/work/llvm/include    -UNDEBUG -fno-exceptions -fno-rtti
> -MMD -MT
> lib/CodeGen/AsmPrinter/CMakeFiles/LLVMAsmPrinter.dir/ARMException.cpp.o -MF
> lib/CodeGen/AsmPrinter/CMakeFiles/LLVMAsmPrinter.dir/ARMException.cpp.o.d
> -o lib/CodeGen/AsmPrinter/CMakeFiles/LLVMAsmPrinter.dir/ARMException.cpp.o
> -c /Users/filcab/work/llvm/lib/CodeGen/AsmPrinter/ARMException.cpp
>
>
> Thank you,
>
>   Filipe
>
> On Wed, Jul 29, 2015 at 3:24 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> 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
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>> _______________________________________________
>> 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/39a1ae4c/attachment.html>

More information about the llvm-commits mailing list