[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/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 {}; }
----------------
zoecarver wrote:
> 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. 
//Please// put the sentinel in-line in the body of the class. Yes, three times.
This test should not rely on the fact that `HasMinusBeginEnd::sentinel` is the same type as `RangeAccessRange::sentinel` — those two types should be //different//. Making them members is the easiest way to ensure that. And also, readability for the maintainer.

You mention below that the `unsigned long` I thought was due to the difference_type` of `forward_iterator` is actually due to the return type of the `operator-` on line 221. This is exactly the kind of subtle detail that is best dealt with by keeping code close together and using member classes. Please do. If you don't, I'll have to do it in a separate followup commit, and I don't want to have to remember that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101079