[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/__ranges/empty.h:48
+
+  struct __fn {
+    template <__member_empty _Tp>
----------------
tcanens wrote:
> 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.
@tcanens: "Expressions don't have reference type" — What if `T` is the type of the expression `foo()`, where `foo` is declared as `int (&foo())[]`? Wouldn't `foo()` have reference type then? Or would you simply say it has type "array of unknown bound" and is an lvalue?
Anyway, libstdc++ and MSVC both agree that in that case `ranges::empty` should SFINAE away, which is the expected/simple/obvious answer anyway.


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp:12
+// UNSUPPORTED: gcc-10
+// XFAIL: msvc && clang
+
----------------
Remove XFAIL here (and throughout); the msvc issue is fixed.


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp:25
+
+static_assert(!std::is_invocable_v<RangeEmptyT, int[]>);
+static_assert( std::is_invocable_v<RangeEmptyT, int[1]>);
----------------
Add:
```
static_assert(!std::is_invocable_v<RangeEmptyT, int(&)[]>);
static_assert(!std::is_invocable_v<RangeEmptyT, int(&&)[]>);
```


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp:71
+  size_t size_;
+  constexpr size_t size() { return size_; }
+};
----------------
`constexpr size_t size() const { return size_; }`


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp:96
+struct sentinel {
+  constexpr bool operator==(std::input_or_output_iterator auto) const { return true; }
+};
----------------
I'm still confused about why `sentinel` needs to be comparable to every-iterator-type-ever.
I would rather see it scoped down to whatever specific type you're planning to compare it to (via nested member types, as shown in another of these reviews).


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp:111-112
+// Int is integer-like, but it is not other_forward_iterator's difference_type.
+constexpr int operator-(const sentinel, const random_access_iterator<int*>) { return 2; }
+constexpr int operator-(const random_access_iterator<int*>, const sentinel) { return 2; }
+static_assert(!std::is_invocable_v<RangeSizeT, InvalidMinusBeginEnd&>);
----------------
You have some pass-by-const-value here. Remove `const`. Also, remove the definition and constexprness from any of these functions that you don't expect to be called.


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/empty.pass.cpp:128
+  friend constexpr sentinel end(DisabledSizeRangeWithBeginEnd) { return {}; }
+  constexpr size_t size() { return 1; }
+};
----------------
`constexpr size_t size() const { return 1; }`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101193