[libc++] Fix bugs in common_iterator; add test coverage.

[• Quuxplusone]

Summary

Eliminates non_default_constructible_iterator and convertible_iterator.

[Mordante]

Can you explain in the commit message why this is removed? It's not clear from the patch why it's a good idea.

Other then that it look good.

[• Quuxplusone]

Also eliminate convertible_iterator since it's unused as well.

[ldionne]

libcxx/test/std/iterators/predef.iterators/iterators.common/ctor.pass.cpp

1

While we're at it, would you mind splitting the tests for these various constructors?

And asking for that made me realize that we're not testing the default constructor at all (we're checking is_default_constructible but we're not actually running it).

34

See, now you understand why I fixed cpp17_input_iterator to *not* be default constructible! 😄

75–76

is_convertible and is_constructible are different in that is_convertible won't work if the conversion/construction is explicit. I think this would be better:

static_assert( std::is_convertible_v<DerivedIt, BaseIt>); // Derived* to Base*
static_assert(!std::is_convertible_v<BaseIt, DerivedIt>); // Base* to Derived*

Unless that isn't what you want (in which case I'd like to understand)?

[• Quuxplusone]

libcxx/test/std/iterators/predef.iterators/iterators.common/ctor.pass.cpp

34

I'll admit, it was convenient. ;)
I think what I wanted here was one default-constructible input iterator and one non-; which could have been cpp17_input_iterator and cpp20_input_iterator; except that cpp20_input_iterator is also not copyable, and I forget but I think that might disqualify it from use with common_iterator.

75–76

I was being maybe too clever omitting "redundant" tests here: I wanted to test that DerivedIt was implicitly convertible to BaseIt, but BaseIt wasn't even explicitly convertible to DerivedIt. I could have (and now, will) test all four permutations:

static_assert( std::is_convertible_v<DerivedIt, BaseIt>); // Derived* to Base*
static_assert( std::is_constructible_v<BaseIt, DerivedIt>); // Derived* to Base*
static_assert(!std::is_convertible_v<BaseIt, DerivedIt>); // Base* to Derived*
static_assert(!std::is_constructible_v<DerivedIt, BaseIt>); // Base* to Derived*

[CaseyCarter]

libcxx/test/std/iterators/predef.iterators/iterators.common/ctor.pass.cpp

34

I think what I wanted here was one default-constructible input iterator and one non-; which could have been cpp17_input_iterator and cpp20_input_iterator; except that cpp20_input_iterator is also not copyable, and I forget but I think that might disqualify it from use with common_iterator.

That's correct: common_iterator<I, S> requires copyable<I> - it was the most expedient fix after relaxing the iterator requirements to allow move-only single-pass iterators. I believe it _could_ support move-only iterators by storing a variant<shared_ptr<I>, S>, but someone would need to flesh out and implement that design and write a paper.

[• Quuxplusone]

Well, this turned into quite the rabbit hole.
My new and improved ctor tests ended up triggering lots of missing constexpr, and then adding the constexpr (and some regression tests) found more bugs. I touched the tests that clearly needed regression tests for the bugs I was fixing. A followup PR should improve the remaining tests (and eliminate the remaining "types.h").

[• Quuxplusone]

Fix LWG3595 while I'm at it.
Poke CI, which is already green.
Ping @ldionne!

[• Quuxplusone]

libcxx/docs/Status/Cxx2bIssues.csv

129 ↗

(On Diff #402169)

Marked https://wg21.link/LWG3574 as Complete in 14.0, also.

[ldionne]

libcxx/test/std/iterators/predef.iterators/iterators.common/ctor_default.pass.cpp

1 ↗

(On Diff #402169)

ctor.default.pass.cpp, ctor.converting.pass.cpp, etc.

libcxx/test/std/iterators/predef.iterators/iterators.common/ctor.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17

 #include "test_macros.h"
 #include "types.h"
 
 template<class I, class S>
 concept ValidCommonIterator = requires {
   typename std::common_iterator<I, S>;
 };
 
-template<class I, class I2>
-concept ConvCtorEnabled = requires(std::common_iterator<I2, sentinel_type<int*>> ci) {
-  std::common_iterator<I, sentinel_type<int*>>(ci);
-};
-
 void test() {
   int buffer[8] = {1, 2, 3, 4, 5, 6, 7, 8};
 
-  static_assert( std::is_default_constructible_v<std::common_iterator<int*, sentinel_type<int*>>>);
-  static_assert(!std::is_default_constructible_v<std::common_iterator<non_default_constructible_iterator<int*>, sentinel_type<int*>>>);
+  static_assert( std::is_default_constructible_v<std::common_iterator<int*, sentinel_wrapper<int*>>>);
+  static_assert(!std::is_default_constructible_v<std::common_iterator<cpp17_input_iterator<int*>, sentinel_wrapper<cpp17_input_iterator<int*>>>>);
 
   // Not copyable:
-  static_assert(!ValidCommonIterator<cpp20_input_iterator<int*>, sentinel_type<int*>>);
+  static_assert(!ValidCommonIterator<cpp20_input_iterator<int*>, sentinel_wrapper<int*>>);
   // Same iter and sent:
   static_assert(!ValidCommonIterator<cpp17_input_iterator<int*>, cpp17_input_iterator<int*>>);
 
   {
-    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});
-
-    assert(*iter1 == 1);
+    using It = cpp17_input_iterator<int*>;
+    using Sent = sentinel_wrapper<It>;
+    auto commonIter1 = std::common_iterator<It, Sent>(It(buffer));
+    auto commonSent1 = std::common_iterator<It, Sent>(Sent(It(buffer + 8)));
     assert(*commonIter1 == 1);
     assert(commonIter1 != commonSent1);
+    assert(std::next(commonIter1, 8) == commonSent1);
   }
   {
-    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});
-
-    assert(*iter1 == 1);
+    using It = forward_iterator<int*>;
+    using Sent = sentinel_wrapper<It>;
+    auto commonIter1 = std::common_iterator<It, Sent>(It(buffer));
+    auto commonSent1 = std::common_iterator<It, Sent>(Sent(It(buffer + 8)));
     assert(*commonIter1 == 1);
     assert(commonIter1 != commonSent1);
+    assert(std::next(commonIter1, 8) == commonSent1);
   }
   {
-    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});
-
-    assert(*iter1 == 1);
+    using It = random_access_iterator<int*>;
+    using Sent = sentinel_wrapper<It>;
+    auto commonIter1 = std::common_iterator<It, Sent>(It(buffer));
+    auto commonSent1 = std::common_iterator<It, Sent>(Sent(It(buffer + 8)));
     assert(*commonIter1 == 1);
     assert(commonIter1 != commonSent1);
+    assert(std::next(commonIter1, 8) == commonSent1);
   }
-
-  // Conversion constructor:
   {
-    convertible_iterator<int*> conv{buffer};
-    auto commonIter1 = std::common_iterator<convertible_iterator<int*>, sentinel_type<int*>>(conv);
-    auto commonIter2 = std::common_iterator<forward_iterator<int*>, sentinel_type<int*>>(commonIter1);
-    assert(*commonIter2 == 1);
-
-    static_assert( ConvCtorEnabled<forward_iterator<int*>, convertible_iterator<int*>>);
-    static_assert(!ConvCtorEnabled<forward_iterator<int*>, random_access_iterator<int*>>);
+    // Test the converting constructor
+    struct Base {};
+    struct Derived : Base {};
+
+    using BaseIt = std::common_iterator<Base*, const Base*>;
+    using DerivedIt = std::common_iterator<Derived*, const Derived*>;
+    static_assert(std::is_convertible_v<DerivedIt, BaseIt>); // Derived* to Base*
+    static_assert(!std::is_constructible_v<DerivedIt, BaseIt>); // Base* to Derived*
+    Derived a[10];
+    auto it = DerivedIt(a); // the iterator type
+    auto jt = BaseIt(it);
+    assert(jt == BaseIt(a));
+    it = DerivedIt((const Derived*)a); // the sentinel type
+    jt = it;
+    assert(jt == BaseIt(a));
   }
 }
 
 int main(int, char**) {
   test();
 
   return 0;
 }

libcxx/test/std/iterators/predef.iterators/iterators.common/types.h

friend constexpr auto operator-(const comparable_iterator& lhs, const simple_iterator<It>& rhs) {
       return lhs.base() - rhs.base();
     }
     friend constexpr auto operator-(const simple_iterator<It>& lhs, const comparable_iterator& rhs) {
       return lhs.base() - rhs.base();
     }
 };
 
 template <class It>
-class convertible_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_;}
-
-    convertible_iterator() = default;
-    explicit constexpr convertible_iterator(It it) : it_(it) {}
-
-    constexpr reference operator*() const {return *it_;}
-
-    constexpr convertible_iterator& operator++() {++it_; return *this;}
-    constexpr convertible_iterator operator++(int)
-        {convertible_iterator tmp(*this); ++(*this); return tmp;}
-
-    operator forward_iterator<It>() const { return forward_iterator<It>(it_); }
-};
-
-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;

libcxx/test/support/test_iterators.h

public:
     TEST_CONSTEXPR It base() const {return it_;} // TODO remove me
     friend TEST_CONSTEXPR It base(const forward_iterator& i) { return i.it_; }
 
     template <class T>
     void operator,(T const &) = delete;
 };
 
 template <class It>
-class non_default_constructible_iterator
-{
-    It it_;
-
-    template <class U> friend class non_default_constructible_iterator;
-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;
-
-    non_default_constructible_iterator() = delete;
-
-    TEST_CONSTEXPR explicit non_default_constructible_iterator(It it) : it_(it) {}
-    template <class U>
-        TEST_CONSTEXPR non_default_constructible_iterator(const non_default_constructible_iterator<U>& u) : it_(u.it_) {}
-
-    TEST_CONSTEXPR reference operator*() const {return *it_;}
-    TEST_CONSTEXPR pointer operator->() const {return it_;}
-
-    TEST_CONSTEXPR_CXX14 non_default_constructible_iterator& operator++() {++it_; return *this;}
-    TEST_CONSTEXPR_CXX14 non_default_constructible_iterator operator++(int) {return non_default_constructible_iterator(it_++);}
-
-    friend TEST_CONSTEXPR bool operator==(const non_default_constructible_iterator& x, const non_default_constructible_iterator& y) {return x.it_ == y.it_;}
-    friend TEST_CONSTEXPR bool operator!=(const non_default_constructible_iterator& x, const non_default_constructible_iterator& y) {return x.it_ != y.it_;}
-
-    template <class T>
-    void operator,(T const &) = delete;
-};
-
-template <class It>
 class bidirectional_iterator
 {
     It it_;
 
     template <class U> friend class bidirectional_iterator;
 public:
     typedef          std::bidirectional_iterator_tag           iterator_category;
     typedef typename std::iterator_traits<It>::value_type      value_type;