[libcxx-commits] [PATCH] D115272: [libc++] [test] Refactor range.prim/empty.pass.cpp

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone marked 5 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp:122
 };
-static_assert(std::is_invocable_v<RangeSizeT, IntPtrBeginAndEnd&>);
 
----------------
var-const wrote:
> Question: why is it okay to remove this test?
`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`.)


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp:125-126
   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()
 
----------------
These comments are backwards. Will fix.


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp:101
 struct BeginEndNotSizedSentinel {
-  friend constexpr forward_iterator<int*> begin(BeginEndNotSizedSentinel) { return {}; }
-  friend constexpr sentinel end(BeginEndNotSizedSentinel) { return {}; }
----------------
ldionne wrote:
> Was `forward_iterator` not relevant for test coverage? I guess we were only relying on the fact that it was not a `sized_range`?
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."


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp:106-114
-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; }
----------------
ldionne wrote:
> I don't understand why this removal doesn't reduce test coverage.
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.


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp:142-146
-struct BeginEndAndConstEmpty {
-  int* begin();
-  int* end();
-  constexpr bool empty() const { return true; }
 };
----------------
ldionne wrote:
> How does this not reduce coverage? Now we don't test with a `empty() const` method anymore.
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);
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115272/new/

https://reviews.llvm.org/D115272