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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
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();
+    }
----------------
Quuxplusone wrote:
> 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.
Good catch, thanks. I'm not going to do `bool(X)` for the overload on L57 because we know that it's a primitive type. 


================
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; }
----------------
Quuxplusone wrote:
> (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); });
> ```
> 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); });

That test will fail to compile, but I get what you mean. Added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101193