[libcxx-commits] [PATCH] D142335: [libc++][ranges] Partially implement `ranges::to`.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__ranges/to.h:97
+        common_range<_Range> &&
+        __iterator_traits_detail::__cpp17_input_iterator<iterator_t<_Range>> &&
+        constructible_from<_Container, iterator_t<_Range>, sentinel_t<_Range>, _Args...>
----------------
ldionne wrote:
> Can you double-check that this is the right concept? And we should also move it outside of that detail namespace -- that's an unusual way to implement an exposition-only concept, but this comes from the very beginning of our ranges implementation. Basically let's clean up all those concepts in `__iterator_traits_detail`.
Interestingly enough, LWG 3733 (https://cplusplus.github.io/LWG/issue3733) has changed this condition since the original paper to not use the `cpp17-input-iterator` concept. Updated.


================
Comment at: libcxx/test/std/containers/associative/from_range_associative_containers.h:208-210
+void test_associative_set_with_input(std::vector<T>&& input) {
+  auto b = Iter(input.data());
+  auto e = Iter(input.data() + input.size());
----------------
ldionne wrote:
> Why not just
> 
> ```
> std::vector<T> const& input
> ```
> 
> ? And then no need to `std::move(b)` and `std::move(e)` below either, I think?
`.data()` would return a const pointer, so the iterator types would have to be parametrized on `const T*` instead of just `T*`. It's doable but I don't think it will be better than the current state.


================
Comment at: libcxx/test/std/containers/associative/from_range_associative_containers.h:259
+
+  // Normal input.
+  test_with_input({0, 5, 12, 7, -1, 8, 26});
----------------
ldionne wrote:
> Should we test with duplicates (here and elsewhere)?
Added simple tests. I made a separate test for each set class and each map class -- all my attempts to generalize turned out to be more trouble than they were worth.


================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp:48
+static_assert(!std::ranges::view<ContainerT>);
+static_assert(HasTo<ContainerT, InputRange>);
+static_assert(!HasTo<test_view<forward_iterator>, InputRange>);
----------------
ldionne wrote:
> I think this is technically ill-formed according to the spec. I wonder whether it makes sense to test the exact way in which it is ill-formed as we do here (probably not).
> 
> Also note that if that's correct and this is ill-formed, then technically we could also be SFINAE friendly as a QOI matter. I don't think we should unless it's simple and other implementations are doing it as well.
Sorry, but at this point I need a reminder on why this is ill-formed.


================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to_std_containers.pass.cpp:153
+template <class K, class V>
+struct std::hash<std::pair<const K, V>> {
+  std::size_t operator()(const std::pair<const K, V>& p) const {
----------------
ldionne wrote:
> Why do you need this?
There's a test below (the one that uses `all_containers`) that uses `std::pair` to be able to convert between map and non-map containers. The specialization is necessary to be able to use `std::pair` with `unordered_set/multiset`. Added a comment to that effect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142335