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

[Tim Song via Phabricator via libcxx-commits]

tcanens added a subscriber: CaseyCarter.
tcanens added inline comments.


================
Comment at: libcxx/include/__ranges/empty.h:38
+  template<class _Tp>
+  concept __can_invoke_size = requires(_Tp&& __t) { ranges::size(__t); };
+
----------------
I'd expect this one and the next to require `!__member_empty`?


================
Comment at: libcxx/include/__ranges/empty.h:48
+
+  struct __fn {
+    template <__member_empty _Tp>
----------------
Quuxplusone wrote:
> 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?
> 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()`.

The difference is that for array of unknown bound of incomplete type, `ranges::begin` / `ranges::end` are IFNDR, but `ranges::empty` is ill-formed DR.

> Do we understand the intent (in all these CPOs) when `T` is a //reference to// an array of unknown bound?

`T` is the type of some expression. Expressions don't have reference type.


================
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();
+    }
----------------
zoecarver wrote:
> cjdb wrote:
> > 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.
> > > and also in the return
> > 
> > This shouldn't be necessary thanks to //`boolean-testable`//. @tcanens I don't think the wording permit us to use it right now; did we miss one in the cleanup?
> 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. 
> > and also in the return
> 
> This shouldn't be necessary thanks to //`boolean-testable`//. @tcanens I don't think the wording permit us to use it right now; did we miss one in the cleanup?

This one doesn't even require the result to be implicitly convertible to `bool`, which is probably why I didn't touch it with //`boolean-testable`//. I don't know what the original intended design is - @CaseyCarter?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101193