On 27/04/2015 13:55, Jonathan Wakely wrote:
> On 22/04/15 22:10 +0200, François Dumont wrote:
>> Hello
>>
>>    I don't know if I am missing something but I think __niter_base 
>> could be simplified to remove usage of _Iter_base. Additionally I 
>> overload it to also remove __normal_iterator layer even if behind a 
>> reverse_iterator or move_iterator, might help compiler to optimize 
>> code, no ? If not, might allow other algo optimization in the future...
>>
>>    I prefered to provide a __make_reverse_iterator to allow the 
>> latter in C++11 and not only in C++14. Is it fine to do it this way 
>> or do you prefer to simply get rid of all this part ?
>
> It's fine to add __make_reverse_iterator but see my comment below.
>
>>    * include/bits/cpp_type_traits.h (__gnu_cxx::__normal_iterator): 
>> Delete.
>
> You're removing __is_normal_iterator not __normal_iterator.
>
>>    * include/bits/stl_algobase.h (std::__niter_base): Adapt.
>>    * include/bits/stl_iterator.h (__make_reverse_iterator): New in 
>> C++11.
>>    (std::__niter_base): Overloads for std::reverse_iterator,
>>    __gnu_cxx::__normal_iterator and std::move_iterator.
>>
>> Tested under Linux x86_64. I checked that std::copy still ends up 
>> calling __builtin_memmove when used on vector iterators.
>>
>> François
>>
>
>> diff --git a/libstdc++-v3/include/bits/stl_algobase.h 
>> b/libstdc++-v3/include/bits/stl_algobase.h
>> index 0bcb133..73eea6b 100644
>> --- a/libstdc++-v3/include/bits/stl_algobase.h
>> +++ b/libstdc++-v3/include/bits/stl_algobase.h
>> @@ -270,17 +270,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>       return __a;
>>     }
>>
>> -  // If _Iterator is a __normal_iterator return its base (a plain 
>> pointer,
>> -  // normally) otherwise return it untouched.  See copy, fill, ... 
>> +  // Fallback implementation of the function used to remove the
>> +  // __normal_iterator wrapper. See copy, fill, ...
>
> It's a bit strange to have a function with no other overloads visible
> described as a fallback. It would be good to say that the other
> definition is in bits/stl_iterator.h
>
>>   template<typename _Iterator>
>> -    struct _Niter_base
>> -    : _Iter_base<_Iterator, __is_normal_iterator<_Iterator>::__value>
>> -    { };
>> -
>> -  template<typename _Iterator>
>> -    inline typename _Niter_base<_Iterator>::iterator_type
>> +    inline _Iterator
>>     __niter_base(_Iterator __it)
>> -    { return std::_Niter_base<_Iterator>::_S_base(__it); }
>> +    { return __it; }
>>
>>   // Likewise, for move_iterator.
>
> This comment no longer makes sense, because you've removed the comment
> on _Niter_base that it referred to. Please restore the original text
> of the _Niter_base comment for _Miter_base.
>
> (Alternatively, could the same simplification be made for
> __miter_base? Do we need _Miter_base<> or just two overloads of
> __miter_base()?)

Definitely, I already have a patch for that.

>
>
>>   template<typename _Iterator>
>> diff --git a/libstdc++-v3/include/bits/stl_iterator.h 
>> b/libstdc++-v3/include/bits/stl_iterator.h
>> index 4a9189e..3aad9f3 100644
>> --- a/libstdc++-v3/include/bits/stl_iterator.h
>> +++ b/libstdc++-v3/include/bits/stl_iterator.h
>> @@ -390,7 +390,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>     { return __y.base() - __x.base(); }
>>   //@}
>>
>> -#if __cplusplus > 201103L
>> +#if __cplusplus == 201103L
>> +  template<typename _Iterator>
>> +    inline reverse_iterator<_Iterator>
>> +    __make_reverse_iterator(_Iterator __i)
>> +    { return reverse_iterator<_Iterator>(__i); }
>> +
>> +# define _GLIBCXX_MAKE_REVERSE_ITERATOR(_Iter) \
>> +  std::__make_reverse_iterator(_Iter)
>> +#elif __cplusplus > 201103L
>> #define __cpp_lib_make_reverse_iterator 201402
>>
>>   // _GLIBCXX_RESOLVE_LIB_DEFECTS
>> @@ -400,6 +408,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>     inline reverse_iterator<_Iterator>
>>     make_reverse_iterator(_Iterator __i)
>>     { return reverse_iterator<_Iterator>(__i); }
>> +
>> +# define _GLIBCXX_MAKE_REVERSE_ITERATOR(_Iter) \
>> +  std::make_reverse_iterator(_Iter)
>> +#endif
>> +
>> +#if __cplusplus >= 201103L
>> +  template<typename _Iterator>
>> +    auto
>> +    __niter_base(reverse_iterator<_Iterator> __it)
>> +    -> 
>> decltype(_GLIBCXX_MAKE_REVERSE_ITERATOR(__niter_base(__it.base())))
>> +    { return 
>> _GLIBCXX_MAKE_REVERSE_ITERATOR(__niter_base(__it.base())); }
>> #endif
>>
>
> It might be simpler to just add __make_reverse_iterator for >= 201103L
> and then always use std::__make_reverse_iterator instead of a macro.
>
> That's similar to what we do for std:__addressof and std:addressof.
>
Ok, attached is the patch I have plan to commit then that I am testing 
at the moment.

François