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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/__ranges/size.h:86-90
+  template <__member_size _Tp>
+  [[nodiscard]] constexpr integral auto operator()(_Tp&& __t) const
+      noexcept(noexcept(_VSTD::forward<_Tp>(__t).size())) {
+    return _VSTD::forward<_Tp>(__t).size();
+  }
----------------
Quuxplusone wrote:
> (A) I see how `integral auto` is mimicking https://eel.is/c++draft/range.prim.size#4 ; however, that seems to inappropriately restrict people from trying to use their own "integer-like" types, at the cost of compile-time-slowness and source-code-size. I propose that you remove the word `integral` from every overload here and below.
> 
> (B) Your `noexcept` specification fails to account for the decay-copy that happens if `__t.size()` returns `const SomeType&`. Admittedly, this is a problem //only// if you remove the `integral` constraint, because decay-copying a primitive integral type can't possibly throw.
> 
> (C) for the record: One might observe that all these conditional-noexcept-specifications violate the spirit of [P0884 "Extending the noexcept Policy"](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0884r0.pdf) and propose removing them; but @tcanens has pointed out that "expression-equivalent to" implies "has the same noexceptness as," so we //are// actually mandated to use (correct) noexcept-specifications throughout basically all of Ranges. (Ugh, but unavoidable.)
> that seems to inappropriately restrict people from trying to use their own "integer-like" types, at the cost of compile-time-slowness and source-code-size.

Only the implementation may provide integer-like types, so this is a non-concern.


================
Comment at: libcxx/include/__ranges/size.h:101
+      noexcept(noexcept(ranges::end(__t) - ranges::begin(__t))) {
+    using _Unsigned = make_unsigned_t<iter_difference_t<decltype(ranges::begin(__t))>>;
+    return static_cast<_Unsigned>(ranges::end(__t) - ranges::begin(__t));
----------------
This can be `range_difference_t<remove_cvref_t<_Tp>>`.


================
Comment at: libcxx/include/__ranges/size.h:105
+
+  size_t operator()(auto &&) const = delete;
+};
----------------
Quuxplusone wrote:
> Could you explain why this `=delete`'d overload is necessary?
> I see how it is //observable// ( https://godbolt.org/z/PGxzf4jGz ) and I assume it is //permitted//, but I don't see why it's //necessary// in libc++. Other implementors (GCC, MSVC) get away without it.
It provides good intrinsic documentation. @zoecarver can you please contrast the diagnostic with/without this deletion, and maybe upload them to a GitHub gist?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101079