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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a comment.

Tests mostly LGTM, but I couldn't find any tests for when member `size` and ADL `size` return signed integers. It's important that signedness is preserved in both cases, so we should add tests if there aren't any.



================
Comment at: libcxx/include/type_traits:2814
 template <class _Tp>
-inline _LIBCPP_INLINE_VISIBILITY
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
 typename decay<_Tp>::type
----------------
No longer necessary (I'd prefer it be added in a separate commit).


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/size.pass.cpp:38
+
+struct SizeFunction {
+  friend constexpr size_t size(SizeFunction) { return 42; }
----------------
Please also test `SizeFunctionRef` and `SizeFunctionRefToConst`.


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/size.pass.cpp:136-139
+// Intentionally disabling "const ConstSizeMemberDisabled". This doesn't disable anything
+// because T is always uncvrefed before being checked.
+template<>
+inline constexpr bool std::disable_sized_range<const ConstSizeMemberDisabled> = true;
----------------
Users can't do this: http://eel.is/c++draft/ranges#range.sized-3.


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/size.pass.cpp:148-153
+struct ConstSizeFunctionDisabled {
+  friend size_t size(const ConstSizeFunctionDisabled) { return 42; }
+};
+
+template<>
+inline constexpr bool std::disable_sized_range<ConstSizeFunctionDisabled> = true;
----------------
This, however, is fine :-)


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/size.pass.cpp:170-176
+struct HasMinusBeginEnd {
+  friend constexpr forward_iterator<int*> begin(HasMinusBeginEnd) { return {}; }
+  friend constexpr sentinel end(HasMinusBeginEnd) { return {}; }
+};
+
+constexpr long operator-(const sentinel, const forward_iterator<int*>) { return 2; }
+constexpr long operator-(const forward_iterator<int*>, const sentinel) { return 2; }
----------------
Please add a test for when the iterator isn't a `forward_iterator`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101079