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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a subscriber: tcanens.
cjdb added a comment.

Implementation LGTM, will look at tests soonish.



================
Comment at: libcxx/include/__ranges/size.h:54
+template <class _Tp>
+concept __size_enabled = !disable_sized_range<remove_cvref_t<_Tp>>;
+
----------------
Thanks for making something that clears up the double negative!


================
Comment at: libcxx/include/__ranges/size.h:72
+concept __difference =
+  __size_enabled<_Tp> && // TODO: maybe not.
+  !__member_size<_Tp> &&
----------------
Yeah, I don't think this one is necessary here.


================
Comment at: libcxx/include/__ranges/size.h:77
+  requires(_Tp&& __t) {
+    // TODO: how do we convert to unsigned?
+    { ranges::begin(__t) } -> forward_iterator;
----------------
See http://eel.is/c++draft/ranges.syn#1.

The result needs to be the `iter_difference_t<_Tp>`, and since I've asserted that we only care about (extended) integral types, we're clear to use `make_unsigned_t<iter_difference_t<_Tp>>`.


================
Comment at: libcxx/include/__ranges/size.h:104
+    // TODO: static cast somewhere here?
+    return _VSTD::__decay_copy(ranges::end(__t) - ranges::begin(__t));
+  }
----------------
@tcanens mentioned in D100255 that `__decay_copy` isn't necessary in the actual function calls.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101079