[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/__iterator/concepts.h:142-145
+template<class _From>
+[[nodiscard]] constexpr auto __to_unsigned_like(_From __x) noexcept {
+  return static_cast<make_unsigned_t<_From>>(__x);
+}
----------------
It seems to me that this function belongs right below `make_unsigned_t`, and not right below `bidirectional_iterator`.


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/size.pass.cpp:47
+
+bool constexpr testArrayType() {
+  int a[4];
----------------
`constexpr bool` (and likewise throughout)
https://quuxplusone.github.io/blog/2021/04/03/static-constexpr-whittling-knife/


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/size.pass.cpp:172
+struct SizeMemberDisabled {
+  size_t size() { return 42; }
+};
----------------
`size_t size() const`


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/size.pass.cpp:182-185
+// 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<ConstSizeMemberDisabled> = true;
----------------
The comment doesn't match the code. I believe you intended to write
```
inline constexpr bool std::disable_sized_range<const ConstSizeMemberDisabled> = true;
```
Also, please rename to something like `s/ConstSizeMemberDisabled/ImproperlyDisabled/`


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/size.pass.cpp:195
+struct ConstSizeFunctionDisabled {
+  friend size_t size(const ConstSizeFunctionDisabled) { return 42; }
+};
----------------
Pass-by-const-value alert!
Either add the `&`, or remove the `const`. (Personally I'd add the `&`.)
https://quuxplusone.github.io/blog/2019/01/03/const-is-a-contract/
That blog post ends with some `git grep` lines; please run them over this and the rest of your patches and see how many other typos they catch.


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/size.pass.cpp:212-222
+struct sentinel {
+  bool operator==(std::input_or_output_iterator auto) const { return true; }
+};
+
+struct HasMinusBeginEnd {
+  friend constexpr forward_iterator<int*> begin(HasMinusBeginEnd) { return {}; }
+  friend constexpr sentinel end(HasMinusBeginEnd) { return {}; }
----------------
Do this instead:
```
struct HasMinusBeginEnd {
  struct sentinel {
    friend bool operator==(sentinel, forward_iterator<int*>);
    friend constexpr long operator-(sentinel, forward_iterator<int*>) { return 2; }
    friend long operator-(forward_iterator<int*>, sentinel);
  };
  friend constexpr forward_iterator<int*> begin(HasMinusBeginEnd) { return {}; }
  friend constexpr sentinel end(HasMinusBeginEnd) { return {}; }
};
```
This keeps everything nicely packaged together, and also indicates (by the presence of function bodies) which functions are actually called by the test, and even (by the presence of `constexpr`) which functions are constexpr-called by the test.

Similarly for the other tests.


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/size.pass.cpp:231
+
+// Int is integer-like, but it is not other_forward_iterator's difference_type.
+constexpr short operator-(const sentinel, const other_forward_iterator) { return 2; }
----------------
Should this comment `s/Int/short/` or do I not understand it? What is capital-i `Int` in this context?


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/size.pass.cpp:238
+
+struct RandomAccesslRange {
+  constexpr random_access_iterator<int*> begin() { return {}; }
----------------
Is the `l` meaningful?


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/size.pass.cpp:252-260
+struct DisabledSizeRangeWithBeginEnd {
+  int buff[8];
+  constexpr int* begin() { return buff; }
+  constexpr int* end() { return buff + 8; }
+  constexpr size_t size() { return 1; }
+};
+
----------------
This is a good test. I'd like to see a similar one where `disable_sized_range` is //not// specialized; then my understanding is that `std::ranges::size(a)` would be supposed to return `a.size()` i.e. `1`, is that right?


================
Comment at: libcxx/test/std/ranges/range.access/range.prim/size.pass.cpp:266
+  // Ensure that this is converted to an *unsigned* type.
+  ASSERT_SAME_TYPE(decltype(std::ranges::size(a)), unsigned long);
+
----------------
This should be `std::make_unsigned_t<std::ptrdiff_t>`, I believe. I don't think we should assume `ptrdiff_t` is invariably `signed long`.
Ditto throughout.
Let's test against `long` (or `short` or whatever) only when it's possible for the reader to grep back and see where that `long` (or `short` or whatever) comes from.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101079