[libcxx-commits] [PATCH] D101193: [libcxx][ranges] Add ranges::empty CPO.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/CMakeLists.txt:44
   __ranges/end.h
   __ranges/enable_borrowed_range.h
   __ranges/size.h
----------------
At some point we should alphabetize `end` vs `ena`.


================
Comment at: libcxx/include/__ranges/empty.h:48
+
+  struct __fn {
+    template <__member_empty _Tp>
----------------
Doesn't clearly implement a case corresponding to the bullet point ["If `T` is an array of unknown bound, `ranges​::​empty(E)` is ill-formed."](https://eel.is/c++draft/range.access#range.prim.empty-2.1)
But I think that bullet point is actually redundant; arrays of unknown bound also don't have `ranges::end(E)` or `ranges::size(E)` or `t.begin()`.
Do we understand the intent (in all these CPOs) when `T` is a //reference to// an array of unknown bound?


================
Comment at: libcxx/include/__ranges/empty.h:50-53
+    [[nodiscard]] constexpr bool operator()(_Tp&& __t) const
+        noexcept(noexcept(_VSTD::forward<_Tp>(__t).empty())) {
+      return _VSTD::forward<_Tp>(__t).empty();
+    }
----------------
As usual, replace `X` with `bool(X)` in the noexcept-spec and also in the return. Also, `__t` should be an lvalue according to "let t be an lvalue that denotes the reified object for E." So:
```
[[nodiscard]] constexpr bool operator()(_Tp&& __t) const
    noexcept(noexcept(bool(__t.empty()))) {
  return __t.empty();
}
```
Optionally, figure out a way to detect the difference here and write a regression test for it. (I say "optionally" because IMHO it doesn't really matter.)

As above so on lines 34 and 57/58: `t` is an lvalue, lose the forwarding.


================
Comment at: libcxx/include/__ranges/empty.h:63-64
+    [[nodiscard]] constexpr bool operator()(_Tp&& __t) const
+        noexcept(noexcept(bool(ranges::end(__t) == ranges::begin(__t)))) {
+      return ranges::end(__t) == ranges::begin(__t);
+    }
----------------



================
Comment at: libcxx/include/ranges:63-64
+#include <__ranges/empty.h>
 #include <__ranges/end.h>
 #include <__ranges/enable_borrowed_range.h>
 #include <__ranges/size.h>
----------------
At some point, alphabetize `end` and `ena`.


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp:24-29
+struct HasMemberAndFunction {
+  constexpr bool empty() { return true; }
+};
+
+// We should never do ADL lookup for std::ranges::empty.
+constexpr bool empty(HasMemberAndFunction) { return false; }
----------------
(A) If you're intending this to be found (or rather not found) by ADL, please make it a hidden friend.
(B) Please make the functions look as much like "realistic/simple code" as you can. In this case, that means marking `empty()` and `size()` as const-qualified member functions throughout. E.g.:
```
struct HasMemberAndFunction {
    constexpr bool empty() const { return true; ]
    friend bool empty(const HasMemberAndFunction&) { return false; }
};
```
And normally I'd say "no constexpr"; but here you need them to be constexpr because you're testing the constexpr-callableness of `ranges::empty`.

Btw, if you ever run into a situation where you need to test //for sure// that some specific function was entered, you can use the old "`was_called = true;`" trick. You just need to make `bool& was_called` a data member, and initialize it in the constructor to refer to a local variable of the function doing the testing. (Global variables can't be modified in constexpr contexts, but locals can.)

Finally, please add a test that //intentionally// has a non-const `size` and `empty`:
```
struct S { int size(); bool empty(); };
static_assert(!requires (const S& s) { std::ranges::size(s); });
static_assert(!requires (const S& s) { std::ranges::empty(s); });
```


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp:40
+struct BoolConvertibleReturnType {
+  constexpr BoolConvertible empty() { return {}; }
+};
----------------
>From context, I suspect you meant to mark this function `noexcept`.


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp:89
+};
+static_assert(!std::is_invocable_v<RangeSizeT, BeginEndNotSizedSentinel&>);
+
----------------
If this was cut-and-pasted from the `ranges::size` tests... should this now say `RangeEmptyT`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101193