[libcxx-commits] [PATCH] D101079: [libcxx][ranges] Add ranges::size CPO.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Overall looks great, with a few comments.

Also:

1. Is there something to update in the Ranges progress document?
2. Can we add conformance tests for the types defined in libc++? `std::vector` & friends.



================
Comment at: libcxx/include/__ranges/size.h:9
+//===----------------------------------------------------------------------===//
+#ifndef _LIBCPP_RANGES_SIZE_H
+#define _LIBCPP_RANGES_SIZE_H
----------------
`_LIBCPP___RANGES_SIZE_H` if we want to be consistent? I agree it's ugly as hell though, so maybe we should revisit that policy! But for now let's be consistent?


================
Comment at: libcxx/include/__ranges/size.h:58
+concept __member_size = __size_enabled<_Tp> && requires(_Tp&& __t) {
+  { _VSTD::__decay_copy(_VSTD::forward<_Tp>(__t).size()) } -> integral;
+};
----------------
I think we need to introduce a proper concept for integer-like types:


```
template <typename _Ip>
concept __integer_like = !same_as<bool, _Ip> && integral<_Ip>;
```

According to the Standard:

> A type `I` other than `cv bool` is integer-like if it models `integral<I>` or if it is an integer-class type.

That would also be an indirection point where we can add our own implementation-defined integer-class type if we have one.

Right now, my understanding is that this concept would be satisfied if `t.size()` returned a `bool`. Instead, we should use `-> __integer_like` so that `bool` is excluded. Can you confirm that it's a problem and add a test that fails when that's the case?

This comment applies to other uses of `integral` below (and so we should add tests for those too).


================
Comment at: libcxx/include/__ranges/size.h:81-89
+  template <class _Tp, size_t _Sz>
+  [[nodiscard]] constexpr size_t operator()(_Tp (&&)[_Sz]) const noexcept {
+    return _Sz;
+  }
+
+  template <class _Tp, size_t _Sz>
+  [[nodiscard]] constexpr size_t operator()(_Tp (&)[_Sz]) const noexcept {
----------------
Just thinking out loud, but this works if you pass a reference to a const array because it'll deduce `_Tp = Foo const` such that the whole signature is `Foo const (&)[_Sz]`?


================
Comment at: libcxx/include/__ranges/size.h:106-107
+      noexcept(noexcept(ranges::end(__t) - ranges::begin(__t))) {
+    using _Unsigned = make_unsigned_t<range_difference_t<remove_cvref_t<_Tp>>>;
+    return static_cast<_Unsigned>(ranges::end(__t) - ranges::begin(__t));
+  }
----------------
Would it be worth creating a `__to_unsigned_like` helper function that we can reuse? I don't know the spec by heart, but I imagine that if they factored out the term of art to-unsigned-like in the spec (https://eel.is/c++draft/ranges#syn-1), it must be because we're re-using it elsewhere?


================
Comment at: libcxx/include/__ranges/size.h:110
+};
+}
+
----------------
`// end namespace __size`


================
Comment at: libcxx/include/__ranges/size.h:113
+inline namespace __cpo {
+inline constexpr const auto size = __size::__fn{};
+} // namespace __cpo
----------------
Indent inside the namespace!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101079