Hide move_iterator ill-form operators

Message ID e6194762-7220-302f-084d-15541a19238d@gmail.com
State New
Headers show
Series
  • Hide move_iterator ill-form operators
Related show

Commit Message

François Dumont May 6, 2019, 5:36 p.m.
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 ?

François

Comments

Jonathan Wakely May 6, 2019, 8:50 p.m. | #1
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()))

Patch

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++); }
 
       _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--); }
 
       _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() )
     { return __x.base() < __y.base(); }
 
   template<typename _Iterator>
-    inline _GLIBCXX17_CONSTEXPR bool
+    inline _GLIBCXX17_CONSTEXPR auto
     operator<(const move_iterator<_Iterator>& __x,
 	      const move_iterator<_Iterator>& __y)
+    -> decltype( __x.base() < __y.base() )
     { return __x.base() < __y.base(); }
 
   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() )
     { return !(__y < __x); }
 
   template<typename _Iterator>
-    inline _GLIBCXX17_CONSTEXPR bool
+    inline _GLIBCXX17_CONSTEXPR auto
     operator<=(const move_iterator<_Iterator>& __x,
 	       const move_iterator<_Iterator>& __y)
+    -> decltype( __x.base() <= __y.base() )
     { return !(__y < __x); }
 
   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() )
     { return __y < __x; }
 
   template<typename _Iterator>
-    inline _GLIBCXX17_CONSTEXPR bool
+    inline _GLIBCXX17_CONSTEXPR auto
     operator>(const move_iterator<_Iterator>& __x,
 	      const move_iterator<_Iterator>& __y)
+    -> decltype( __x.base() > __y.base() )
     { return __y < __x; }
 
   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() )
     { return !(__x < __y); }
 
   template<typename _Iterator>
-    inline _GLIBCXX17_CONSTEXPR bool
+    inline _GLIBCXX17_CONSTEXPR auto
     operator>=(const move_iterator<_Iterator>& __x,
 	       const move_iterator<_Iterator>& __y)
+    -> decltype( __x.base() >= __y.base() )
     { return !(__x < __y); }
 
   // DR 685.
diff --git a/libstdc++-v3/testsuite/24_iterators/move_iterator/operator_neg.cc b/libstdc++-v3/testsuite/24_iterators/move_iterator/operator_neg.cc
new file mode 100644
index 00000000000..7b09425358e
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/move_iterator/operator_neg.cc
@@ -0,0 +1,73 @@ 
+// Copyright (C) 2019 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do compile { target c++11 xfail *-*-* } }
+
+#include <forward_list>
+
+using ite_t =
+  std::move_iterator<typename std::forward_list<int>::iterator>;
+using const_ite_t =
+  std::move_iterator<typename std::forward_list<int>::const_iterator>;
+
+using lower_type =
+  decltype( std::declval<ite_t>() < std::declval<ite_t>() ); // { dg-error "no match for 'operator<'" }
+
+// { dg-error "no match for 'operator<'" "" { target *-*-* } 1195 }
+
+using heterogeneous_lower_type =
+  decltype( std::declval<const_ite_t>() < std::declval<ite_t>() ); // { dg-error "no match for 'operator<'" }
+
+// { dg-error "no match for 'operator<'" "" { target *-*-* } 1202 }
+
+using heterogeneous_lower_equal_type =
+  decltype( std::declval<const_ite_t>() <= std::declval<ite_t>() ); // { dg-error "no match for 'operator<='" }
+
+// { dg-error "no match for 'operator<='" "" { target *-*-* } 1209 }
+
+using lower_equal_type =
+  decltype( std::declval<ite_t>() <= std::declval<ite_t>() ); // { dg-error "no match for 'operator<='" }
+
+// { dg-error "no match for 'operator<='" "" { target *-*-* } 1216 }
+
+using heterogeneous_greater_type =
+  decltype( std::declval<const_ite_t>() > std::declval<ite_t>() ); // { dg-error "no match for 'operator>'" }
+
+// { dg-error "no match for 'operator>'" "" { target *-*-* } 1223 }
+
+using greater_type =
+  decltype( std::declval<ite_t>() > std::declval<ite_t>() ); // { dg-error "no match for 'operator>'" }
+
+// { dg-error "no match for 'operator>'" "" { target *-*-* } 1230 }
+
+using heterogeneous_greater_equal_type =
+  decltype( std::declval<const_ite_t>() >= std::declval<ite_t>() ); // { dg-error "no match for 'operator>='" }
+
+// { dg-error "no match for 'operator>='" "" { target *-*-* } 1237 }
+
+using greater_equal_type =
+  decltype( std::declval<ite_t>() >= std::declval<ite_t>() ); // { dg-error "no match for 'operator>='" }
+
+// { dg-error "no match for 'operator>='" "" { target *-*-* } 1244 }
+
+using diff_type =
+  decltype( std::declval<ite_t>() - std::declval<ite_t>() ); // { dg-error "no match for 'operator-'" }
+
+// { dg-error "no match for 'operator-'" "" { target *-*-* } 1252 }
+
+using integral_plus_ite_type =
+  decltype( 1 + std::declval<ite_t>() );