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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver 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();
+  }
----------------
cjdb wrote:
> 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.
See http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1522r1.pdf, specifically para 2.2
> We do this in such a way that (a) only implementors of the Standard Library can define and use such types for now, and (b) is forward ABI compatible...

Also, this: https://reviews.llvm.org/D100080#inline-952255

---

The Standard requires that these are only integer like types, and tells us that we get to decide what that means. I think, if for no other reason than we should strive for maximum compatibility (i.e., someone shouldn't be able to write some integer type and use it in their program such that libc++ compiles and libstdc++ or msvc doesn't) we should require these types are `integral`. Also, I think there is an argument for correctness here.

The "compile-time-slowness and source-code-size" are extremely minimal here. The former likely can't be measured over noise. 


================
Comment at: libcxx/include/__ranges/size.h:105
+
+  size_t operator()(auto &&) const = delete;
+};
----------------
cjdb wrote:
> 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?
This is a so called poison pill. I could be persuaded to remove it, but I think it makes the error message nicer for users. I'd like to point out that other implementations //do// use it. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101079