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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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();
+  }
----------------
(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.)


================
Comment at: libcxx/include/__ranges/size.h:105
+
+  size_t operator()(auto &&) const = delete;
+};
----------------
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.


================
Comment at: libcxx/include/__ranges/size.h:116
+
+#undef _LIBCPP_NOEXCEPT_RETURN
+
----------------
You can delete this line now. Thanks for removing this macro.


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/size.compile.pass.cpp:122-123
+static_assert( std::is_invocable_v<RangeSizeT, InvalidReturnTypeFunction (&)[4]>);
+static_assert(!std::is_invocable_v<RangeSizeT, ConvertableReturnTypeMember>);
+static_assert(!std::is_invocable_v<RangeSizeT, ConvertableReturnTypeFunction>);
+
----------------
Throughout: s/Convertable/Convertible/


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/size.compile.pass.cpp:126
+struct SizeMemberDisabled {
+  constexpr size_t size() { return 42; }
+};
----------------
(A) Remove `constexpr`. Remove the function body. `size_t size() const;` is sufficient.
(B) I notice you didn't const-qualify this member; I assume it was unintentional; but this is a good place to ask, what //is// supposed to happen if a type has a non-const-qualified `size()`? What does that imply for the truthiness of `sized_range<{const,} T{&,&&,}>`?


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/size.compile.pass.cpp:192-196
+  IntPtrBeginAndEnd b;
+  assert(std::ranges::size(b) == 8);
+
+  DisabledSizeRangeWithBeginEnd c;
+  assert(std::ranges::size(c) == 8);
----------------
You use `assert` and have a `main`, but this file's name is `size.compile.pass.cpp`.
I suppose we need yet another CI step to prevent people from putting `main`s in their `.compile.pass.cpp` tests... and probably to prevent using the identifier-token `assert` in those files, either.


================
Comment at: libcxx/test/support/test_range.h:20
 struct sentinel {
-  bool operator==(std::input_or_output_iterator auto) const;
+  bool operator==(std::input_or_output_iterator auto) const { return true; }
 };
----------------
This function body should be unnecessary. If it's needed by some test, that test is wrong.
If for testing purposes we //want// a sentinel type that always produces an empty range, then we should name it something clearer than `struct sentinel`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101079