Reverse range adapter
Filipe Cabecinhas
filcab at gmail.com
Wed Jul 29 17:02:13 PDT 2015
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
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/711dcc24/attachment.html>
More information about the llvm-commits
mailing list