[llvm] r243581 - Reapply "Add reverse(ContainerTy) range adapter."

[Duncan P. N. Exon Smith via llvm-commits]

[resending to the "new" llvm-commits!]

[A little necromancy here...]

I think Charles was correct.

The attached rbegin.patch adds const-qualified rbegin() overloads for ReverseOnlyVector and BidirectionalVector.

It fails to compile, because the `has_rbegin` type trait returns false when rbegin() is const-overloaded.  reverse() tries to call begin()/end() on ReverseOnlyVector and they don't exist.

If you remove ReverseOnlyVector from the list of types to test, then it compiles.  However, all three types tested fail the new HasRbegin test.

I hit this because I'm working on a patch that makes the ilist reverse iterators incompatible with std::reverse_iterator.

I haven't made time to fix the type trait yet (or I'd commit this patch with the fix), but I thought I'd post the problem in the meantime.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: rbegin.patch
Type: application/octet-stream
Size: 1873 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160817/509cceba/attachment.obj>
-------------- next part --------------


> On 2015-Jul-29, at 20:32, Pete Cooper <peter_cooper at apple.com> wrote:
> 
> 
> 
> Sent from my iPhone
> 
>> On Jul 29, 2015, at 7:56 PM, Charles Davis <cdavis5x at gmail.com> wrote:
>> 
>> 
>>> On Jul 29, 2015, at 6:48 PM, Pete Cooper <peter_cooper at apple.com> wrote:
>>> 
>>> Should be fixed by r243581.  Sorry for the breakage.
>> That fixed the build error, alright (and thanks for the quick turnaround!), but it doesn’t really address the “has_rbegin not quite working right” concern I brought up. I guess it’s OK for `std::vector`, but I’m just afraid we might get strange or broken behavior from this for other containers with overloaded-for-const `rbegin()`/`rend()` methods where `std::reverse_iterator` might not work right. I don’t know if there are any that we need to worry about, though.
> Sorry, should have answered this part before. I was in a rush to get the fix in.
> 
> So I believe it'll be ok because the ContainerTy itself knows if it's const or not so will select the correct method. From your log:
> 
> 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(
> 
> So hopefully it's all good, but thanks for looking in to it.
> 
> Pete
>>