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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
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 {}; }
----------------
Quuxplusone wrote:
> 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.
I don't want to have to define sentinel three times, so I'm going to keep it out-of-line. But I'll make the comparison operator a hidden friend. 


================
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);
+
----------------
Quuxplusone wrote:
> 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.
```
struct HasMinusBeginEnd {
  friend constexpr forward_iterator<int*> begin(HasMinusBeginEnd) { return {}; }
  friend constexpr sentinel end(HasMinusBeginEnd) { return {}; }
};

constexpr long operator-(const sentinel, const forward_iterator<int*>) { return 2; }
constexpr long operator-(const forward_iterator<int*>, const sentinel) { return 2; }
```

So `long` is hardcoded in this case (as the minus operator's return type).


================
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);
+
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > 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.
> ```
> struct HasMinusBeginEnd {
>   friend constexpr forward_iterator<int*> begin(HasMinusBeginEnd) { return {}; }
>   friend constexpr sentinel end(HasMinusBeginEnd) { return {}; }
> };
> 
> constexpr long operator-(const sentinel, const forward_iterator<int*>) { return 2; }
> constexpr long operator-(const forward_iterator<int*>, const sentinel) { return 2; }
> ```
> 
> So `long` is hardcoded in this case (as the minus operator's return type).
>  I don't think we should assume ptrdiff_t is invariably signed long.

If that assumption is ever made in this test, it will be caught by the windows and arm bots. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101079