[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 ↗

(On Diff #392544)

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.)

125–127

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; }
 };
 struct BoolConvertibleReturnType {
   constexpr BoolConvertible empty() noexcept { return {}; }
 };
-
 static_assert(!noexcept(std::ranges::empty(BoolConvertibleReturnType())));
 
 constexpr bool testEmptyMember() {
   HasMemberAndFunction a;
   assert(std::ranges::empty(a) == true);
 
   BoolConvertibleReturnType b;
   assert(std::ranges::empty(b) == true);
constexpr bool testUsingRangesSize() {
   SizeFunction c{1};
   assert(std::ranges::empty(c) == false);
   SizeFunction d{0};
   assert(std::ranges::empty(d) == 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 {}; }
-};
-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&>);
-
-// This type will use ranges::size.
-struct IntPtrBeginAndEnd {
-  int buff[8];
-  constexpr int* begin() { return buff; }
-  constexpr int* end() { return buff + 8; }
+  constexpr int *begin() const { return nullptr; }
+  constexpr auto end() const { return sentinel_wrapper<int*>(nullptr); }
 };
-static_assert(std::is_invocable_v<RangeSizeT, IntPtrBeginAndEnd&>);
+static_assert( std::ranges::forward_range<BeginEndNotSizedSentinel>);
+static_assert(!std::ranges::sized_range<BeginEndNotSizedSentinel>);
 
-// 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 {}; }
+  constexpr sentinel_wrapper<int*> end() const { return {}; }
+  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 int *end() const { return 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);  // using begin()==end()
+  assert(std::ranges::empty(std::as_const(e)) == true);  // using empty()
 
   return true;
 }
 
 int main(int, char**) {
   testEmptyMember();
   static_assert(testEmptyMember());
 
   testUsingRangesSize();
   static_assert(testUsingRangesSize());
 
   testBeginEqualsEnd();
   static_assert(testBeginEqualsEnd());
 
   return 0;
 }