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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

Thanks! I already reviewed this a bunch and IMO this is starting to look really good!



================
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());
----------------
Why not just

```
std::vector<T> const& input
```

? And then no need to `std::move(b)` and `std::move(e)` below either, I think?


================
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});
----------------
Should we test with duplicates (here and elsewhere)?


================
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 {
----------------
Why do you need this?


================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to_std_containers.pass.cpp:221
+    /*
+    types::for_each(container_adaptors<int>{}, []<class From>() {
+      types::for_each(container_adaptors<double>{}, []<class To>() {
----------------
Looks like a leftover!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142335