[Bug libstdc++/94354] New: std::reverse_iterator comparison operators defined incorrectly

[Bug libstdc++/94354] New: std::reverse_iterator comparison operators defined incorrectly

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94354

            Bug ID: 94354
           Summary: std::reverse_iterator comparison operators defined
                    incorrectly
           Product: gcc
           Version: 10.0
            Status: UNCONFIRMED
          Keywords: rejects-valid
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: redi at gcc dot gnu.org
  Target Milestone: ---

We define the comparison operators for reverse_iterator slightly differently to
how the standard specifies them, which means we incorrectly reject this:

#include <iterator>

struct Iter
{
  using iterator_category = std::random_access_iterator_tag;
  using value_type = int;
  using pointer = int*;
  using reference = int&;
  using difference_type = std::ptrdiff_t;

  Iter();

  Iter& operator++();
  Iter operator++(int);
  Iter& operator--();
  Iter operator--(int);
  int& operator*() const;
  int* operator->() const;

  int& operator[](difference_type) const;

  Iter& operator+=(difference_type);
  Iter& operator-=(difference_type);

  friend Iter operator+(Iter, difference_type);
  friend Iter operator+(difference_type, Iter);
  friend Iter operator-(Iter, difference_type);
  friend difference_type operator-(Iter, Iter);

  friend bool operator==(Iter, Iter);
  friend bool operator!=(Iter, Iter);
  friend bool operator<(Iter, Iter);
  friend bool operator>(Iter, Iter);
  friend bool operator<=(Iter, Iter);
  friend bool operator>=(Iter, Iter);
};

bool operator!=(Iter, long*);

std::reverse_iterator<Iter> l{Iter()};
std::reverse_iterator<long*> r{nullptr};

#if __cplusplus > 201703L
static_assert( std::random_access_iterator<Iter> );
static_assert( std::random_access_iterator<long*> );
#endif

bool b1 = l.base() != r.base();  // OK

bool b2 = l != r;


reviter.cc:50:13:   in 'constexpr' expansion of 'std::operator!=<Iter, long
int*>(l, r)'
/home/jwakely/gcc/10/include/c++/10.0.1/bits/stl_iterator.h:375:25: error: no
match for 'operator==' (operand types are
'std::reverse_iterator<Iter>::iterator_type' {aka 'Iter'} and
'std::reverse_iterator<long int*>::iterator_type' {aka 'long int*'})
  375 |     { return __x.base() == __y.base(); }
      |              ~~~~~~~~~~~^~~~~~~~~~~~~


The reverse_iterator's preconditions are met (both Iter and long* are random
access iterators) and so is the constraint for operator!=

Constraints: x.base() != y.base() is well-formed and convertible to bool.

But it fails because we define operator!= as !(l == r) which uses == on the
base iterator types, and they only support !=

I think the testcase should be valid for C++11 and later, as per
https://cplusplus.github.io/LWG/lwg-defects.html#280
(and also C++98 if we treat that as a DR).

[Bug libstdc++/94354] std::reverse_iterator comparison operators defined incorrectly

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94354

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2020-03-27
     Ever confirmed|0                           |1
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org
             Status|UNCONFIRMED                 |ASSIGNED

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
We also retain the pre-LWG280 homogeneous overloads:

  template<typename _Iterator>
    inline _GLIBCXX17_CONSTEXPR bool
    operator==(const reverse_iterator<_Iterator>& __x,
               const reverse_iterator<_Iterator>& __y)
    { return __x.base() == __y.base(); }

I don't have a demo, but this might be non-conforming for cases where we
compare reverse_iterator<T> and something convertible to that type.

[Bug libstdc++/94354] std::reverse_iterator comparison operators defined incorrectly

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94354

--- Comment #2 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:81a8d137c22953df2ea046466c62cd26c0dba103

commit r10-7434-g81a8d137c22953df2ea046466c62cd26c0dba103
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Mar 27 23:21:58 2020 +0000

    libstdc++: Add remaining C++20 changes to iterator adaptors

    This adds the missing parts of P0896R4 to reverse_iterator and
    move_iterator, so that they meet the C++20 requirements. This should be
    the last piece of P0896R4, meaning ranges support is now complete.

    The PR 94354 bug with reverse_iterator's comparisons is fixed for C++20
    only, but that change should be extended to C++11, C++14 and C++17 modes
    in stage 1.

            * include/bits/stl_iterator.h (reverse_iterator::iterator_concept)
            (reverse_iterator::iterator_category): Define for C++20.
            (reverse_iterator): Define comparison operators correctly for
C++20.
            (__normal_iterator): Add constraints to comparison operators for
C++20.
            (move_iterator::operator++(int)) [__cpp_lib_concepts]: Define new
            overload for input iterators.
            (move_iterator): Add constraints to comparison operators for C++20.
            Define operator<=> for C++20.
            * testsuite/24_iterators/move_iterator/input_iterator.cc: New test.
            * testsuite/24_iterators/move_iterator/move_only.cc: New test.
            * testsuite/24_iterators/move_iterator/rel_ops_c++20.cc: New test.
            * testsuite/24_iterators/reverse_iterator/rel_ops_c++20.cc: New
test.

[Bug libstdc++/94354] std::reverse_iterator comparison operators defined incorrectly

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94354

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |11.0

[Bug libstdc++/94354] std::reverse_iterator comparison operators defined incorrectly

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94354

--- Comment #3 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:979e89a9a94f66241fa8355e2b2e8f4a680c83e1

commit r11-672-g979e89a9a94f66241fa8355e2b2e8f4a680c83e1
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed May 27 21:58:56 2020 +0100

    libstdc++: Fix std::reverse_iterator comparisons (PR 94354)

    The std::reverse_iterator comparisons have always been implemented only
    in terms of equality and less than. In C++98 that made no difference for
    reasonable code, because when the underlying operators are the same type
    they are required to support all comparisons anyway.

    But since LWG 280 it's possible to compare reverse_iterator<X> and
    reverse_iterator<Y>, and comparisons between X and Y might not support
    the full set of equality and relational operators. This means that it
    matters whether we implement operator!= as x.base() != y.base() or
    !(x.base() == y.base()), and the current implementation is
    non-conforming.

    This was already fixed in GCC 10.1 for C++20, this change also fixes it
    for all other -std modes.

            PR libstdc++/94354
            * include/bits/stl_iterator.h (reverse_iterator): Fix comparison
            operators to use the correct operations on the underlying
            iterators.
            * testsuite/24_iterators/reverse_iterator/rel_ops.cc: New test.

[Bug libstdc++/94354] std::reverse_iterator comparison operators defined incorrectly

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94354

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed for GCC 11