Re: Hide move_iterator ill-form operators

[Jonathan Wakely <jwakely at redhat dot com>]

On 06/05/19 19:36 +0200, François Dumont wrote:
> Hi
>
>     This is another attempt to make adapter iterator types operators 
> undefined when underlying iterator type doesn't support it. For the 
> move_iterator it is rather easy and even already done for the 
> operator- so I just generalize it to comparison operators. It doesn't 
> cover all operators of course but it is still better than current 
> situation.
>
>     * include/bits/stl_iterator.h (move_iterator<>::operator++(int)):
>     Simplify implementation using underlying iterator type same
>     post-increment operator.
>     (move_iterator<>::operator--(int)):
>     Simplify implementation using underlying iterator type same
>     post-decrement operator.
>     (move_iterator<>::operator<(const move_iterator<>&,
>     const move_iterator<>&): Define return type as return type of the same
>     expression on underlying iterator type.
>     (move_iterator<>::operator<=(const move_iterator<>&,
>     const move_iterator<>&): Likewise.
>     (move_iterator<>::operator>(const move_iterator<>&,
>     const move_iterator<>&): Likewise.
>     (move_iterator<>::operator>=(const move_iterator<>&,
>     const move_iterator<>&): Likewise.
>     * testsuite/24_iterators/move_iterator/operator_neg.cc: New.
>
>     Ok to commit or should the Standard be amended first ?

Not OK.

The C++2a draft already solves the same problem, but differently.
Please follow the draft standard, instead of inventing something
different.

> François

> diff --git a/libstdc++-v3/include/bits/stl_iterator.h b/libstdc++-v3/include/bits/stl_iterator.h
> index 47be1a9dbcd..c1bbc75ca43 100644
> --- a/libstdc++-v3/include/bits/stl_iterator.h
> +++ b/libstdc++-v3/include/bits/stl_iterator.h
> @@ -1121,11 +1121,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
>       _GLIBCXX17_CONSTEXPR move_iterator
>       operator++(int)
> -      {
> -	move_iterator __tmp = *this;
> -	++_M_current;
> -	return __tmp;
> -      }
> +      { return move_iterator(_M_current++); }

This is not what C++2a says.

>       _GLIBCXX17_CONSTEXPR move_iterator&
>       operator--()
> @@ -1136,11 +1132,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
>       _GLIBCXX17_CONSTEXPR move_iterator
>       operator--(int)
> -      {
> -	move_iterator __tmp = *this;
> -	--_M_current;
> -	return __tmp;
> -      }
> +      { return move_iterator(_M_current--); }

This is not what C++2a says.

>       _GLIBCXX17_CONSTEXPR move_iterator
>       operator+(difference_type __n) const
> @@ -1197,51 +1189,59 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     { return !(__x == __y); }
>
>   template<typename _IteratorL, typename _IteratorR>
> -    inline _GLIBCXX17_CONSTEXPR bool
> +    inline _GLIBCXX17_CONSTEXPR auto
>     operator<(const move_iterator<_IteratorL>& __x,
> 	      const move_iterator<_IteratorR>& __y)
> +    -> decltype( __x.base() < __y.base() )

This is wrong, it needs to return bool, e.g.

   -> decltype(bool(__x.base() < __y.base()))