[libcxx-commits] [PATCH] D100269: [libcxx][ranges] adds `ranges::range`, `ranges::common_range`, and range aliases

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/__ranges/concepts.h:38
+
+  // `iterator_t` defined in <__ranges/begin.h>
+
----------------
Quuxplusone wrote:
> cjdb wrote:
> > ldionne wrote:
> > > That's kinda weird, would it be possible to define it here instead?
> > Your options are:
> >   * it lives here exclusively, `__ranges/access.h` (`__ranges/end.h` in current D100255) needs to use `decltype(ranges::begin(declval<_Rp&>()))` wherever it currently uses `iterator_t`
> >   * it lives in `__ranges/access.h` exclusively, comment remains
> >   * it lives in both
> FWIW, I'd look at the definitions on cppreference:
> - `iterator_t` is the type of `ranges::begin`, so it belongs next to `begin`
> - `sentinel_t` is the type of `ranges::end`, so it belongs next to `end`
> - `range_size_t`is the type of `ranges::size`, so it belongs next to `size`
> The other four typedefs probably might as well go into `<ranges>` until we have a reason to move them elsewhere.
> FWIW, I'd look at the definitions on cppreference:

Having written a fair chunk of the ranges section of cppreference, I can confidently say it's designed for consumers of C++, not for implementers, so different groupings make sense.

> * `iterator_t` is the type of `ranges::begin`, so it belongs next to `begin`
> * `sentinel_t` is the type of `ranges::end`, so it belongs next to `end`
> * `range_size_t`is the type of `ranges::size`, so it belongs next to `size`

This is a good suggestion (even though cppreference doesn't group them like this), but I'd prefer to group them as per the standard's synopsis.

> The other four typedefs probably might as well go into `<ranges>` until we have a reason to move them elsewhere.

That would make using them in `<ranges/*>` very awkward.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100269