[libc++] Fix common_iterator for output_iterators

We were missing a constraint in common_iterator's iterator_traits and
we were eagerly instantiating iter_value_t even when invalid.

Thanks to Casey Carter for finding this bug.

Differential Revision: https://reviews.llvm.org/D117449

diff --git a/libcxx/include/__iterator/common_iterator.h b/libcxx/include/__iterator/common_iterator.h
index df4c7bd..605071d 100644 (file)
--- a/libcxx/include/__iterator/common_iterator.h
+++ b/libcxx/include/__iterator/common_iterator.h
@@ -29,6 +29,11 @@ _LIBCPP_BEGIN_NAMESPACE_STD
 
 #if !defined(_LIBCPP_HAS_NO_RANGES)
 
+template<class _Iter>
+concept __can_use_postfix_proxy =
+  constructible_from<iter_value_t<_Iter>, iter_reference_t<_Iter>> &&
+  move_constructible<iter_value_t<_Iter>>;
+
 template<input_or_output_iterator _Iter, sentinel_for<_Iter> _Sent>
   requires (!same_as<_Iter, _Sent> && copyable<_Iter>)
 class common_iterator {
@@ -54,10 +59,6 @@ class common_iterator {
       : __value(_VSTD::forward<iter_reference_t<_Iter>>(__x)) {}
 
   public:
-    constexpr static bool __valid_for_iter =
-      constructible_from<iter_value_t<_Iter>, iter_reference_t<_Iter>> &&
-      move_constructible<iter_value_t<_Iter>>;
-
     constexpr const iter_value_t<_Iter>& operator*() const noexcept {
       return __value;
     }
@@ -148,7 +149,7 @@ public:
       ++*this;
       return __tmp;
     } else if constexpr (requires (_Iter& __i) { { *__i++ } -> __referenceable; } ||
-                         !__postfix_proxy::__valid_for_iter) {
+                         !__can_use_postfix_proxy<_Iter>) {
       return _VSTD::__unchecked_get<_Iter>(__hold_)++;
     } else {
       __postfix_proxy __p(**this);
@@ -261,7 +262,7 @@ struct __arrow_type_or_void<_Iter, _Sent> {
     using type = decltype(declval<const common_iterator<_Iter, _Sent>&>().operator->());
 };
 
-template<class _Iter, class _Sent>
+template<input_iterator _Iter, class _Sent>
 struct iterator_traits<common_iterator<_Iter, _Sent>> {
   using iterator_concept = _If<forward_iterator<_Iter>,
                                forward_iterator_tag,
@@ -275,7 +276,6 @@ struct iterator_traits<common_iterator<_Iter, _Sent>> {
   using reference = iter_reference_t<_Iter>;
 };
 
-
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
 
 _LIBCPP_END_NAMESPACE_STD

diff --git a/libcxx/test/std/iterators/predef.iterators/iterators.common/iterator_traits.compile.pass.cpp b/libcxx/test/std/iterators/predef.iterators/iterators.common/iterator_traits.compile.pass.cpp
index 384c8c7..c55285b 100644 (file)
--- a/libcxx/test/std/iterators/predef.iterators/iterators.common/iterator_traits.compile.pass.cpp
+++ b/libcxx/test/std/iterators/predef.iterators/iterators.common/iterator_traits.compile.pass.cpp
@@ -14,9 +14,23 @@
 
 #include <iterator>
 
+#include <cstddef>
+#include "test_iterators.h"
 #include "test_macros.h"
 #include "types.h"
 
+template<class T>
+concept HasIteratorConcept = requires { typename T::iterator_concept; };
+
+struct NonVoidOutputIterator {
+    using value_type = int;
+    using difference_type = std::ptrdiff_t;
+    const NonVoidOutputIterator& operator*() const;
+    NonVoidOutputIterator& operator++();
+    NonVoidOutputIterator& operator++(int);
+    void operator=(int) const;
+};
+
 void test() {
   {
     using Iter = simple_iterator<int*>;
@@ -43,17 +57,30 @@ void test() {
     static_assert(!std::same_as<IterTraits::pointer, int*>);
     static_assert(std::same_as<IterTraits::reference, int>);
   }
+  // Test with an output_iterator that has a void value_type
   {
-    using Iter = non_const_deref_iterator<int*>;
+    using Iter = output_iterator<int*>;
     using CommonIter = std::common_iterator<Iter, sentinel_type<int*>>;
     using IterTraits = std::iterator_traits<CommonIter>;
 
-    static_assert(std::same_as<IterTraits::iterator_concept, std::input_iterator_tag>);
-    static_assert(std::same_as<IterTraits::iterator_category, std::input_iterator_tag>);
-    static_assert(std::same_as<IterTraits::value_type, int>);
+    static_assert(!HasIteratorConcept<IterTraits>);
+    static_assert(std::same_as<IterTraits::iterator_category, std::output_iterator_tag>);
+    static_assert(std::same_as<IterTraits::value_type, void>);
     static_assert(std::same_as<IterTraits::difference_type, std::ptrdiff_t>);
     static_assert(std::same_as<IterTraits::pointer, void>);
-    static_assert(std::same_as<IterTraits::reference, int&>);
+    static_assert(std::same_as<IterTraits::reference, void>);
+  }
+  // Test with an output_iterator that has a non-void value_type
+  {
+    using CommonIter = std::common_iterator<NonVoidOutputIterator, sentinel_wrapper<NonVoidOutputIterator>>;
+    using IterTraits = std::iterator_traits<CommonIter>;
+
+    static_assert(!HasIteratorConcept<IterTraits>);
+    static_assert(std::same_as<IterTraits::iterator_category, std::output_iterator_tag>);
+    static_assert(std::same_as<IterTraits::value_type, void>);
+    static_assert(std::same_as<IterTraits::difference_type, std::ptrdiff_t>);
+    static_assert(std::same_as<IterTraits::pointer, void>);
+    static_assert(std::same_as<IterTraits::reference, void>);
   }
   {
     using Iter = cpp17_input_iterator<int*>;

diff --git a/libcxx/test/std/iterators/predef.iterators/iterators.common/plus_plus.pass.cpp b/libcxx/test/std/iterators/predef.iterators/iterators.common/plus_plus.pass.cpp
index f3c1b77..257c7ff 100644 (file)
--- a/libcxx/test/std/iterators/predef.iterators/iterators.common/plus_plus.pass.cpp
+++ b/libcxx/test/std/iterators/predef.iterators/iterators.common/plus_plus.pass.cpp
@@ -21,11 +21,10 @@
 struct Incomplete;
 
 void test() {
-  int buffer[8] = {1, 2, 3, 4, 5, 6, 7, 8};
-
   // Reference: http://eel.is/c++draft/iterators.common#common.iter.nav-5
   // Case 2: can-reference
   {
+    int buffer[8] = {1, 2, 3, 4, 5, 6, 7, 8};
     auto iter1 = simple_iterator<int*>(buffer);
     auto commonIter1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
     auto commonSent1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(sentinel_type<int*>{buffer + 8});
@@ -43,6 +42,7 @@ void test() {
 
   // Case 2: can-reference
   {
+    int buffer[8] = {1, 2, 3, 4, 5, 6, 7, 8};
     auto iter1 = value_iterator<int*>(buffer);
     auto commonIter1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
     auto commonSent1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(sentinel_type<int*>{buffer + 8});
@@ -60,6 +60,7 @@ void test() {
 
   // Case 3: postfix-proxy
   {
+    int buffer[8] = {1, 2, 3, 4, 5, 6, 7, 8};
     auto iter1 = void_plus_plus_iterator<int*>(buffer);
     auto commonIter1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
     auto commonSent1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(sentinel_type<int*>{buffer + 8});
@@ -77,13 +78,13 @@ void test() {
 
   // Case 2: where this is not referencable or move constructible
   {
+    int buffer[8] = {1, 2, 3, 4, 5, 6, 7, 8};
     auto iter1 = value_type_not_move_constructible_iterator<int*>(buffer);
     auto commonIter1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
     auto commonSent1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(sentinel_type<int*>{buffer + 8});
 
     commonIter1++;
-    // Note: postfix operator++ returns void.
-    // assert(*(commonIter1++) == 1);
+    ASSERT_SAME_TYPE(decltype(commonIter1++), void);
     assert(*commonIter1 == 2);
     assert(*(++commonIter1) == 3);
     assert(*commonIter1 == 3);
@@ -97,6 +98,7 @@ void test() {
 
   // Case 2: can-reference
   {
+    int buffer[8] = {1, 2, 3, 4, 5, 6, 7, 8};
     auto iter1 = cpp17_input_iterator<int*>(buffer);
     auto commonIter1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
     auto commonSent1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(sentinel_type<int*>{buffer + 8});
@@ -114,6 +116,7 @@ void test() {
 
   // Case 1: forward_iterator
   {
+    int buffer[8] = {1, 2, 3, 4, 5, 6, 7, 8};
     auto iter1 = forward_iterator<int*>(buffer);
     auto commonIter1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
     auto commonSent1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(sentinel_type<int*>{buffer + 8});
@@ -131,6 +134,7 @@ void test() {
 
   // Case 1: forward_iterator
   {
+    int buffer[8] = {1, 2, 3, 4, 5, 6, 7, 8};
     auto iter1 = random_access_iterator<int*>(buffer);
     auto commonIter1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(iter1);
     auto commonSent1 = std::common_iterator<decltype(iter1), sentinel_type<int*>>(sentinel_type<int*>{buffer + 8});
@@ -145,6 +149,31 @@ void test() {
     }
     assert(commonIter1 == commonSent1);
   }
+
+  // Increment a common_iterator<output_iterator>: iter_value_t is not always valid for
+  // output iterators (it isn't for our test output_iterator). This is worth testing
+  // because it gets tricky when we define operator++(int).
+  {
+    int buffer[] = {0, 1, 2, 3, 4};
+    using Common = std::common_iterator<output_iterator<int*>, sentinel_type<int*>>;
+    auto iter = Common(output_iterator<int*>(buffer));
+    auto sent = Common(sentinel_type<int*>{buffer + 5});
+
+    *iter++ = 90;
+    assert(buffer[0] == 90);
+
+    *iter = 91;
+    assert(buffer[1] == 91);
+
+    *++iter = 92;
+    assert(buffer[2] == 92);
+
+    iter++;
+    iter++;
+    assert(iter != sent);
+    iter++;
+    assert(iter == sent);
+  }
 }
 
 int main(int, char**) {

diff --git a/libcxx/test/std/iterators/predef.iterators/iterators.common/types.h b/libcxx/test/std/iterators/predef.iterators/iterators.common/types.h
index 03b94f6..76981c1 100644 (file)
--- a/libcxx/test/std/iterators/predef.iterators/iterators.common/types.h
+++ b/libcxx/test/std/iterators/predef.iterators/iterators.common/types.h
@@ -157,30 +157,6 @@ public:
     }
 };
 
-template <class It>
-class non_const_deref_iterator
-{
-    It it_;
-
-public:
-    typedef          std::input_iterator_tag                   iterator_category;
-    typedef typename std::iterator_traits<It>::value_type      value_type;
-    typedef typename std::iterator_traits<It>::difference_type difference_type;
-    typedef It                                                 pointer;
-    typedef typename std::iterator_traits<It>::reference       reference;
-
-    constexpr It base() const {return it_;}
-
-    non_const_deref_iterator() = default;
-    explicit constexpr non_const_deref_iterator(It it) : it_(it) {}
-
-    constexpr reference operator*() {return *it_;} // Note: non-const.
-
-    constexpr non_const_deref_iterator& operator++() {++it_; return *this;}
-    constexpr non_const_deref_iterator operator++(int)
-        {non_const_deref_iterator tmp(*this); ++(*this); return tmp;}
-};
-
 template<class T>
 struct sentinel_type {
   T base;