[libc++] [test] Refactor range.prim/empty.pass.cpp

[• Quuxplusone]

Summary

This PR now contains three single-file diffs, each orthogonal to the other two, each NFC, each to be landed in a separate commit.

[libc++] [test] Refactor range.prim/empty.pass.cpp

No decrease in test coverage intended. The original goal here
was just to get rid of the global name sentinel so that we can
rename the sentinel_wrapper in "test_iterators.h" to sentinel;
but then I took a closer look at the offending tests and saw
that some of them probably weren't testing what they intended.

[libc++] [test_iterators] Make all ADL base() functions into hidden friends. NFCI.

This follows up on my addition of base(cpp20_input_iterator) in D115177, making all the ADL base() functions consistent.
Also, align cpp20_input_iterator with the other test iterators' style.

[libc++] [test] Use sized_sentinel<int*> in range.prim/ssize.pass.cpp.

[• Quuxplusone]

Actually, since I've got a ton of these little diffs sitting around, let me roll three orthogonal diffs into one Differential...

[ldionne]

FWIW, it's usually easier to instead create three differential revisions and link them in the UI as parent/children.

libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp

101

Was forward_iterator not relevant for test coverage? I guess we were only relying on the fact that it was not a sized_range?

106–114

I don't understand why this removal doesn't reduce test coverage.

142–146

How does this not reduce coverage? Now we don't test with a empty() const method anymore.

[var-const]

libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp

122

Question: why is it okay to remove this test?

libcxx/test/std/ranges/range.access/range.prim/ssize.pass.cpp

43–46

Nit: is there a reason not to make it a member variable?

[• Quuxplusone]

libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp

101

Right — I take BeginEndNotSizedSentinel to mean "we're gonna call begin() == end(), and the sentinel is not a sized sentinel." (If it were sized, then ranges::size would be well-formed and thus we'd end up calling ranges::size instead of begin() == end(). So its being not-sized is important.)
If it's not sized and not a forward iterator, then we (that is, ranges::empty) can't even call begin() == end() because we don't want to invalidate its begin().
I've now added a (passing) test case for "if the iterators are just input iterators, then ranges::empty(e) is ill-formed."

106–114

On old line 114, we're verifying that ranges::size(b) would be ill-formed if we were to call it on old line 153. What actually happens on line 153 is that ranges::empty(b) calls b.begin() == b.end() to find out if the range is empty, because what we have here is not a sized sentinel — this is the same case tested with BeginEndNotSizedSentinel on new lines 118–119.

122

IntPtrBeginAndEnd tests the case where ranges::empty(e) will do ranges::size(e), so we look in testUsingRangesSize()... and hmm, indeed there's no test case in there for the plain old sized-sentinel case (merely test cases for member and ADL size() functions). I guess I should add back one of those.
(The counterargument is that we already have tests for all the different ways to enable ranges::size(e)... in size.pass.cpp. So repeating all those tests here is a little silly. But it's easy to add, so I'm just gonna do it. I'll call it BeginEndSizedSentinel.)

142–144

These comments are backwards. Will fix.

142–146

In that case we would expect ranges::empty(x) to call x.empty(), so you'll find that up in testEmptyMember (which hasn't changed much):

struct HasMemberAndFunction {
  constexpr bool empty() const { return true; }
  // We should never do ADL lookup for std::ranges::empty.
  friend bool empty(const HasMemberAndFunction&) { return false; }
};
~~~
  HasMemberAndFunction a;
  assert(std::ranges::empty(a) == true);

[• Quuxplusone]

Review comments. Mark a TODO (which I have a patch for now).

[ldionne]

This LGTM now.

[• Quuxplusone]

Merged as f638c4d6e4a28fbd5508f853a8715cc92bb66d48, c19d8f955ac3a807978c199fdc4262680d3ad5b2, 67c43d87cd53fdbd75dea6da8dbbd0762357864a.

libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp

struct HasMemberAndFunction {
   constexpr bool empty() const { return true; }
   // We should never do ADL lookup for std::ranges::empty.
   friend bool empty(const HasMemberAndFunction&) { return false; }
 };
 
 struct BadReturnType {
   BadReturnType empty() { return {}; }
 };
-static_assert(!std::is_invocable_v<RangeEmptyT, BadReturnType>);
+static_assert(!std::is_invocable_v<RangeEmptyT, BadReturnType&>);
 
 struct BoolConvertible {
-  constexpr operator bool() noexcept(false) { return true; }
+  constexpr /*TODO: explicit*/ operator bool() noexcept(false) { return true; }
 };
 struct BoolConvertibleReturnType {
   constexpr BoolConvertible empty() noexcept { return {}; }
 };
-
 static_assert(!noexcept(std::ranges::empty(BoolConvertibleReturnType())));
 
+struct InputIterators {
+  cpp17_input_iterator<int*> begin() const;
+  cpp17_input_iterator<int*> end() const;
+};
+static_assert(std::is_same_v<decltype(InputIterators().begin() == InputIterators().end()), bool>);
+static_assert(!std::is_invocable_v<RangeEmptyT, const InputIterators&>);
+
 constexpr bool testEmptyMember() {
   HasMemberAndFunction a;
   assert(std::ranges::empty(a) == true);
 
   BoolConvertibleReturnType b;
   assert(std::ranges::empty(b) == true);
 
   return true;
 }
 
 struct SizeMember {
   size_t size_;
   constexpr size_t size() const { return size_; }
 };
 
 struct SizeFunction {
   size_t size_;
   friend constexpr size_t size(SizeFunction sf) { return sf.size_; }
 };
 
+struct BeginEndSizedSentinel {
+  constexpr int *begin() const { return nullptr; }
+  constexpr auto end() const { return sized_sentinel<int*>(nullptr); }
+};
+static_assert(std::ranges::forward_range<BeginEndSizedSentinel>);
+static_assert(std::ranges::sized_range<BeginEndSizedSentinel>);
+
 constexpr bool testUsingRangesSize() {
   SizeMember a{1};
   assert(std::ranges::empty(a) == false);
   SizeMember b{0};
   assert(std::ranges::empty(b) == true);
 
   SizeFunction c{1};
   assert(std::ranges::empty(c) == false);
   SizeFunction d{0};
   assert(std::ranges::empty(d) == true);
 
+  BeginEndSizedSentinel e;
+  assert(std::ranges::empty(e) == true);
+
   return true;
 }
 
-struct other_forward_iterator : forward_iterator<int*> { };
-
-struct sentinel {
-  constexpr bool operator==(std::input_or_output_iterator auto) const { return true; }
-};
-
 struct BeginEndNotSizedSentinel {
-  friend constexpr forward_iterator<int*> begin(BeginEndNotSizedSentinel) { return {}; }
-  friend constexpr sentinel end(BeginEndNotSizedSentinel) { return {}; }
+  constexpr int *begin() const { return nullptr; }
+  constexpr auto end() const { return sentinel_wrapper<int*>(nullptr); }
 };
-static_assert(!std::is_invocable_v<RangeSizeT, BeginEndNotSizedSentinel&>);
-
-struct InvalidMinusBeginEnd {
-  friend constexpr random_access_iterator<int*> begin(InvalidMinusBeginEnd) { return {}; }
-  friend constexpr sentinel end(InvalidMinusBeginEnd) { return {}; }
-};
-
-// Int is integer-like, but it is not other_forward_iterator's difference_type.
-constexpr short operator-(sentinel, random_access_iterator<int*>) { return 2; }
-constexpr short operator-(random_access_iterator<int*>, sentinel) { return 2; }
-static_assert(!std::is_invocable_v<RangeSizeT, InvalidMinusBeginEnd&>);
+static_assert( std::ranges::forward_range<BeginEndNotSizedSentinel>);
+static_assert(!std::ranges::sized_range<BeginEndNotSizedSentinel>);
 
-// This type will use ranges::size.
-struct IntPtrBeginAndEnd {
-  int buff[8];
-  constexpr int* begin() { return buff; }
-  constexpr int* end() { return buff + 8; }
-};
-static_assert(std::is_invocable_v<RangeSizeT, IntPtrBeginAndEnd&>);
-
-// size is disabled here, and it isn't sized_sentinel_for, so we have to compare begin
-// and end again.
+// size is disabled here, so we have to compare begin and end.
 struct DisabledSizeRangeWithBeginEnd {
-  friend constexpr forward_iterator<int*> begin(DisabledSizeRangeWithBeginEnd) { return {}; }
-  friend constexpr sentinel end(DisabledSizeRangeWithBeginEnd) { return {}; }
-  constexpr size_t size() const { return 1; }
+  constexpr int *begin() const { return nullptr; }
+  constexpr auto end() const { return sentinel_wrapper<int*>(nullptr); }
+  size_t size() const;
 };
-
-template <>
+template<>
 inline constexpr bool std::ranges::disable_sized_range<DisabledSizeRangeWithBeginEnd> = true;
-static_assert(!std::is_invocable_v<RangeSizeT, DisabledSizeRangeWithBeginEnd&>);
+static_assert(std::ranges::contiguous_range<DisabledSizeRangeWithBeginEnd>);
+static_assert(!std::ranges::sized_range<DisabledSizeRangeWithBeginEnd>);
 
 struct BeginEndAndEmpty {
-  int* begin();
-  int* end();
-  constexpr bool empty() { return true; }
-};
-
-struct BeginEndAndConstEmpty {
-  int* begin();
-  int* end();
-  constexpr bool empty() const { return true; }
+  constexpr int *begin() const { return nullptr; }
+  constexpr auto end() const { return sentinel_wrapper<int*>(nullptr); }
+  constexpr bool empty() { return false; }
 };
 
 constexpr bool testBeginEqualsEnd() {
   BeginEndNotSizedSentinel a;
   assert(std::ranges::empty(a) == true);
 
-  InvalidMinusBeginEnd b;
-  assert(std::ranges::empty(b) == true);
-
-  IntPtrBeginAndEnd c;
-  assert(std::ranges::empty(c) == false);
-
   DisabledSizeRangeWithBeginEnd d;
   assert(std::ranges::empty(d) == true);
 
   BeginEndAndEmpty e;
-  assert(std::ranges::empty(e) == true);
-
-  BeginEndAndConstEmpty f;
-  assert(std::ranges::empty(f) == true);
-  assert(std::ranges::empty(std::as_const(f)) == true);
+  assert(std::ranges::empty(e) == false); // e.empty()
+  assert(std::ranges::empty(std::as_const(e)) == true); // e.begin() == e.end()
 
   return true;
 }
 
 int main(int, char**) {
   testEmptyMember();
   static_assert(testEmptyMember());
 
   testUsingRangesSize();
   static_assert(testUsingRangesSize());
 
   testBeginEqualsEnd();
   static_assert(testBeginEqualsEnd());
 
   return 0;
 }

libcxx/test/std/ranges/range.access/range.prim/ssize.pass.cpp

 static_assert( std::is_invocable_v<RangeSSizeT, int (&&)[1]>);
 static_assert( std::is_invocable_v<RangeSSizeT, int (&)[1]>);
 
 static_assert(std::semiregular<std::remove_cv_t<RangeSSizeT>>);
 
 struct SizeMember {
   constexpr size_t size() { return 42; }
 };
-
-static_assert(!std::is_invocable_v<RangeSSizeT, const SizeMember>);
+static_assert(!std::is_invocable_v<decltype(std::ranges::ssize), const SizeMember&>);
 
 struct SizeFunction {
   friend constexpr size_t size(SizeFunction) { return 42; }
 };
 
 struct SizeFunctionSigned {
   friend constexpr std::ptrdiff_t size(SizeFunctionSigned) { return 42; }
 };
 
-struct sentinel {
-  bool operator==(std::input_or_output_iterator auto) const { return true; }
-};
-
-struct RandomAccesslRange {
-  constexpr random_access_iterator<int*> begin() { return {}; }
-  constexpr sentinel end() { return {}; }
+struct SizedSentinelRange {
+  int data_[2] = {};
+  constexpr int *begin() { return data_; }
+  constexpr auto end() { return sized_sentinel<int*>(data_ + 2); }
 };
 
-constexpr std::ptrdiff_t operator-(const sentinel, const random_access_iterator<int*>) { return 2; }
-constexpr std::ptrdiff_t operator-(const random_access_iterator<int*>, const sentinel) { return 2; }
-
 struct ShortUnsignedReturnType {
   constexpr unsigned short size() { return 42; }
 };
 
 // size_t changes depending on the platform.
 using SignedSizeT = std::make_signed_t<size_t>;
 
 constexpr bool test() {
   int a[4];
 
   assert(std::ranges::ssize(a) == 4);
   ASSERT_SAME_TYPE(decltype(std::ranges::ssize(a)), SignedSizeT);
 
   assert(std::ranges::ssize(SizeMember()) == 42);
   ASSERT_SAME_TYPE(decltype(std::ranges::ssize(SizeMember())), SignedSizeT);
 
   assert(std::ranges::ssize(SizeFunction()) == 42);
   ASSERT_SAME_TYPE(decltype(std::ranges::ssize(SizeFunction())), SignedSizeT);
 
   assert(std::ranges::ssize(SizeFunctionSigned()) == 42);
   ASSERT_SAME_TYPE(decltype(std::ranges::ssize(SizeFunctionSigned())), std::ptrdiff_t);
 
-  RandomAccesslRange b;
+  SizedSentinelRange b;
   assert(std::ranges::ssize(b) == 2);
   ASSERT_SAME_TYPE(decltype(std::ranges::ssize(b)), std::ptrdiff_t);
 
   // This gets converted to ptrdiff_t because it's wider.
   ShortUnsignedReturnType c;
   assert(std::ranges::ssize(c) == 42);
   ASSERT_SAME_TYPE(decltype(std::ranges::ssize(c)), ptrdiff_t);
 

libcxx/test/support/test_iterators.h

template <class U>
         TEST_CONSTEXPR_CXX14 output_iterator(const output_iterator<U>& u) :it_(u.it_) {}
 
     TEST_CONSTEXPR_CXX14 reference operator*() const {return *it_;}
 
     TEST_CONSTEXPR_CXX14 output_iterator& operator++() {++it_; return *this;}
     TEST_CONSTEXPR_CXX14 output_iterator operator++(int)
         {output_iterator tmp(*this); ++(*this); return tmp;}
 
+    friend TEST_CONSTEXPR It base(const output_iterator& i) { return i.it_; }
+
     template <class T>
     void operator,(T const &) = delete;
 };
 
 // This is the Cpp17InputIterator requirement as described in Table 87 ([input.iterators]),
 // formerly known as InputIterator prior to C++20.
 template <class It, class ItTraits = It>
 class cpp17_input_iterator
public:
     TEST_CONSTEXPR_CXX14 cpp17_input_iterator operator++(int)
         {cpp17_input_iterator tmp(*this); ++(*this); return tmp;}
 
     friend TEST_CONSTEXPR_CXX14 bool operator==(const cpp17_input_iterator& x, const cpp17_input_iterator& y)
         {return x.it_ == y.it_;}
     friend TEST_CONSTEXPR_CXX14 bool operator!=(const cpp17_input_iterator& x, const cpp17_input_iterator& y)
         {return !(x == y);}
 
+    friend TEST_CONSTEXPR It base(const cpp17_input_iterator& i) { return i.it_; }
+
     template <class T>
     void operator,(T const &) = delete;
 };
 
 template <class T, class TV, class U, class UV>
 bool operator==(const cpp17_input_iterator<T, TV>& x, const cpp17_input_iterator<U, UV>& y)
 {
     return x.base() == y.base();
public:
     TEST_CONSTEXPR_CXX14 forward_iterator operator++(int)
         {forward_iterator tmp(*this); ++(*this); return tmp;}
 
     friend TEST_CONSTEXPR_CXX14 bool operator==(const forward_iterator& x, const forward_iterator& y)
         {return x.it_ == y.it_;}
     friend TEST_CONSTEXPR_CXX14 bool operator!=(const forward_iterator& x, const forward_iterator& y)
         {return !(x == y);}
 
+    friend TEST_CONSTEXPR It base(const forward_iterator& i) { return i.it_; }
+
     template <class T>
     void operator,(T const &) = delete;
 };
 
 template <class T, class U>
 TEST_CONSTEXPR_CXX14
 bool operator==(const forward_iterator<T>& x, const forward_iterator<U>& y)
 {
public:
     TEST_CONSTEXPR_CXX14 bidirectional_iterator& operator++() {++it_; return *this;}
     TEST_CONSTEXPR_CXX14 bidirectional_iterator operator++(int)
         {bidirectional_iterator tmp(*this); ++(*this); return tmp;}
 
     TEST_CONSTEXPR_CXX14 bidirectional_iterator& operator--() {--it_; return *this;}
     TEST_CONSTEXPR_CXX14 bidirectional_iterator operator--(int)
         {bidirectional_iterator tmp(*this); --(*this); return tmp;}
 
+    friend TEST_CONSTEXPR It base(const bidirectional_iterator& i) { return i.it_; }
+
     template <class T>
     void operator,(T const &) = delete;
 };
 
 template <class T, class U>
 TEST_CONSTEXPR_CXX14
 bool operator==(const bidirectional_iterator<T>& x, const bidirectional_iterator<U>& y)
 {
public:
     friend TEST_CONSTEXPR_CXX14 random_access_iterator operator+(difference_type n, random_access_iterator x)
         {x += n; return x;}
     TEST_CONSTEXPR_CXX14 random_access_iterator& operator-=(difference_type n) {return *this += -n;}
     TEST_CONSTEXPR_CXX14 random_access_iterator operator-(difference_type n) const
         {random_access_iterator tmp(*this); tmp -= n; return tmp;}
 
     TEST_CONSTEXPR_CXX14 reference operator[](difference_type n) const {return it_[n];}
 
+    friend TEST_CONSTEXPR It base(const random_access_iterator& i) { return i.it_; }
+
     template <class T>
     void operator,(T const &) = delete;
 };
 
 template <class T, class U>
 TEST_CONSTEXPR_CXX14
 bool operator==(const random_access_iterator<T>& x, const random_access_iterator<U>& y)
 {
public:
     difference_type operator==(const contiguous_iterator& x, const contiguous_iterator& y) {
         return x.base() == y.base();
     }
     friend TEST_CONSTEXPR_CXX14
     difference_type operator!=(const contiguous_iterator& x, const contiguous_iterator& y) {
         return x.base() != y.base();
     }
 
+    friend TEST_CONSTEXPR It base(const contiguous_iterator& i) { return i.it_; }
+
     template <class T>
     void operator,(T const &) = delete;
 };
 #endif
 
-template <class Iter>
-TEST_CONSTEXPR_CXX14 Iter base(output_iterator<Iter> i) { return i.base(); }
-
-template <class Iter>
-TEST_CONSTEXPR_CXX14 Iter base(cpp17_input_iterator<Iter> i) { return i.base(); }
-
-template <class Iter>
-TEST_CONSTEXPR_CXX14 Iter base(forward_iterator<Iter> i) { return i.base(); }
-
-template <class Iter>
-TEST_CONSTEXPR_CXX14 Iter base(bidirectional_iterator<Iter> i) { return i.base(); }
-
-template <class Iter>
-TEST_CONSTEXPR_CXX14 Iter base(random_access_iterator<Iter> i) { return i.base(); }
-
-#if TEST_STD_VER > 17
-template <class Iter>
-TEST_CONSTEXPR_CXX14 Iter base(contiguous_iterator<Iter> i) { return i.base(); }
-#endif
-
-template <class Iter>    // everything else
+template <class Iter> // ADL base() for everything else (including pointers)
 TEST_CONSTEXPR_CXX14 Iter base(Iter i) { return i; }
 
 template <typename T>
 struct ThrowingIterator {
     typedef std::bidirectional_iterator_tag iterator_category;
     typedef ptrdiff_t                       difference_type;
     typedef const T                         value_type;
     typedef const T *                       pointer;

 private:
     const T *begin_;
     const T *end_;
     const T *current_;
 };
 
 #ifdef TEST_SUPPORTS_RANGES
 
-template <class I>
-struct cpp20_input_iterator {
-    using value_type = std::iter_value_t<I>;
-    using difference_type = std::iter_difference_t<I>;
-    using iterator_concept = std::input_iterator_tag;
+template <class It>
+class cpp20_input_iterator
+{
+    It it_;
 
-    cpp20_input_iterator() = delete;
+public:
+    using value_type = std::iter_value_t<It>;
+    using difference_type = std::iter_difference_t<It>;
+    using iterator_concept = std::input_iterator_tag;
 
+    constexpr explicit cpp20_input_iterator(It it) : it_(it) {}
     cpp20_input_iterator(cpp20_input_iterator&&) = default;
     cpp20_input_iterator& operator=(cpp20_input_iterator&&) = default;
+    constexpr decltype(auto) operator*() const { return *it_; }
+    constexpr cpp20_input_iterator& operator++() { ++it_; return *this; }
+    constexpr void operator++(int) { ++it_; }
 
-    cpp20_input_iterator(cpp20_input_iterator const&) = delete;
-    cpp20_input_iterator& operator=(cpp20_input_iterator const&) = delete;
+    constexpr const It& base() const& { return it_; }
+    constexpr It base() && { return std::move(it_); }
 
-    explicit constexpr cpp20_input_iterator(I base) : base_(std::move(base)) {}
-
-    constexpr decltype(auto) operator*() const { return *base_; }
-
-    constexpr cpp20_input_iterator& operator++() {
-        ++base_;
-        return *this;
-    }
-
-    constexpr void operator++(int) { ++base_; }
-
-    constexpr const I& base() const& { return base_; }
-
-    constexpr I base() && { return std::move(base_); }
-
-    friend constexpr I base(const cpp20_input_iterator& i) { return i.base_; }
+    friend constexpr It base(const cpp20_input_iterator& i) { return i.it_; }
 
     template <class T>
     void operator,(T const &) = delete;
-
-private:
-    I base_ = I();
 };
 
 template <std::input_or_output_iterator I>
 struct iterator_concept {
     using type = std::output_iterator_tag;
 };
 
 template <std::input_iterator I>
public:
     constexpr three_way_contiguous_iterator& operator+=(difference_type n) {it_ += n; return *this;}
     constexpr auto operator+(difference_type n) const {auto tmp = *this; tmp += n; return tmp;}
     friend constexpr auto operator+(difference_type n, three_way_contiguous_iterator x) {x += n; return x;}
     constexpr three_way_contiguous_iterator& operator-=(difference_type n) {return *this += -n;}
     constexpr auto operator-(difference_type n) const {auto tmp = *this; tmp -= n; return tmp;}
 
     constexpr reference operator[](difference_type n) const {return it_[n];}
 
-    template <class T>
-    void operator,(T const &) = delete;
-
     friend constexpr
     difference_type operator-(const three_way_contiguous_iterator& x, const three_way_contiguous_iterator& y) {
         return x.base() - y.base();
     }
 
     friend auto operator<=>(const three_way_contiguous_iterator&, const three_way_contiguous_iterator&) = default;
+
+    template <class T>
+    void operator,(T const &) = delete;
 };
 
 #endif // TEST_STD_VER > 17 && defined(__cpp_lib_concepts)
 
 #endif // SUPPORT_TEST_ITERATORS_H