[libcxx-commits] [PATCH] D104262: [libcxx][ranges] Add contiguous_range.

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/test/std/ranges/range.req/range.refinements/contiguous_range.compile.pass.cpp:42-46
+struct BadDataMember {
+  int *begin();
+  int *end();
+  char *data();
+};
----------------
Quuxplusone wrote:
> I was initially confused by the name, as this class has //no// data members... but then I saw: the trick is that `X::data()` member function has a return type that is different from `std::add_pointer_t<ranges::range_reference_t<X>>`.
> I think it'd be worth replacing this test (or at augmenting it) with both of these:
> ```
> struct WrongConstness {
>     const int *begin() const;
>     const int *end() const;
>     int *data() const;
> };
> static_assert(std::ranges::random_access_range<WrongConstness>);
> static_assert(!std::ranges::contiguous_range<WrongConstness>);
> 
> struct NotPtr {
>     contiguous_iterator<const int*> begin() const;
>     contiguous_iterator<const int*> end() const;
>     contiguous_iterator<const int*> data() const;
> };
> static_assert(std::ranges::random_access_range<NotPtr>);
> static_assert(!std::ranges::contiguous_range<NotPtr>);
> ```
> I was initially confused by the name, as this class has //no// data members... but then I saw: the trick is that `X::data()` member function has a return type that is different from `std::add_pointer_t<ranges::range_reference_t<X>>`.

I propose renaming to `BadDataMemberFunction`.

We should also have a check for when a range can satisfy `contiguous_range<R>`, but not `contiguous_range<R const>`.


================
Comment at: libcxx/test/std/ranges/range.req/range.refinements/subsumption.compile.pass.cpp:28
 template<std::ranges::range R>
-requires std::input_iterator<std::ranges::iterator_t<R> >
+requires std::input_iterator<std::ranges::iterator_t<R>>
 [[nodiscard]] constexpr bool check_input_range_subsumption() {
----------------
Quuxplusone wrote:
> Whitespace: Please indent `requires` by one level (here and throughout). E.g.
> ```
> template<...>
>   requires ...
> bool foo() {
>   return ...
> }
> ```
> It'd be nice to remove the [[nodiscard]]s while you're at it.
@ldionne [[ https://reviews.llvm.org/D99691#inline-938990 | left it up to me ]] to decide how libc++ formats requires clauses, and [[ https://reviews.llvm.org/D99691#inline-939014 | I decided that they shouldn't be indented ]].


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104262