[libcxx-commits] [PATCH] D100269: [libcxx] adds concept `range` and range aliases

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In general looks good. Some minor remarks, but would like to see it pass the CI.



================
Comment at: libcxx/include/CMakeLists.txt:157
   ranges
-  regex
   scoped_allocator
----------------
This seems unintentional.


================
Comment at: libcxx/include/__ranges/concepts.h:29
+namespace ranges {
+// [range.range]
+template <class _Tp>
----------------
Is the synopsis in the `<ranges>` header?


================
Comment at: libcxx/include/__ranges/concepts.h:41
+
+template <range R>
+using range_difference_t = iter_difference_t<iterator_t<R> >;
----------------
Please _Uglify. Note more to _Uglify below.


================
Comment at: libcxx/include/__ranges/concepts.h:43
+using range_difference_t = iter_difference_t<iterator_t<R> >;
+
+template <range R>
----------------
Shouldn't
```
template<sized_­range R>
    using range_size_t = decltype(ranges::size(declval<R&>()));
```
be included here? Or is it somewhere else? If it's somewhere else can you add a comment like you did for `iterator_t`?


================
Comment at: libcxx/test/std/containers/unord/unord.multimap/range_concept_conformance.compile.pass.cpp:19
+
+static_assert(std::ranges::range<std::unordered_multimap<int, int> const>);
----------------
Why no non-const version?


================
Comment at: libcxx/test/std/strings/basic.string/range_concept_conformance.compile.pass.cpp:22
+static_assert(std::ranges::range<std::string>);
+static_assert(std::ranges::range<std::string const>);
----------------
Can you also test `std::basic_string` with other character types, `wchar_t`, `char8_t`, `char16_t`, and `char32_t`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100269