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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/test/std/containers/sequences/forwardlist/forwardlist.cons/from_range.pass.cpp:12
+  void operator()() const {
+    test_sequence_container<std::forward_list, int, Iter, Sent, Alloc>([](const auto&) {
+    });
----------------
Comment here


================
Comment at: libcxx/test/std/containers/sequences/list/list.cons/from_range.pass.cpp:13
+    test_sequence_container<std::list, int, Iter, Sent, Alloc>([](const auto&) {
+    });
+  }
----------------
Here I think it would be good to add 

```
[](const auto&) {
  // no additional validation needed for std::list
}
```

Otherwise it looks like an oversight.


================
Comment at: libcxx/test/std/containers/sequences/vector.bool/construct_from_range.pass.cpp:21
+int main(int, char**) {
+  for_all_iterators_and_allocators<bool, TestVector>();
+  test_exception_safety_throwing_allocator<std::vector, bool>();
----------------
```
for_all_iterators_and_allocators<bool>([]<class Iter, class Sent, class Alloc>() {
  // ...
});
```


================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to.ctrs.pass.cpp:1
+#include <algorithm>
+#include <cassert>
----------------
Seems like this can be removed.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/from_range.pass.cpp:12
+  void operator()() const {
+  char array[] = {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'a', 'b', 'c', 'd', 'e', 'f', 'g'};
+    auto b = Iter(array);
----------------
Same comment about different sizes. Also would be nice to cover the SSO and the long string representation.


================
Comment at: libcxx/test/support/from_range_associative_containers.h:44
+void test_associative_map() {
+  using T = std::pair<const K, V>;
+
----------------
That's more explicit and makes the test easier to read IMO


================
Comment at: libcxx/test/support/from_range_associative_containers.h:46
+
+  T array[] = {T{K{1}, V{1}}};
+  auto b = Iter(array);
----------------
We should test with an empty range and also a couple of non-zero sizes.


================
Comment at: libcxx/test/support/from_range_helpers.h:81-105
+template <class T, template <class I, class S, class A> class Func>
+void for_all_iterators_and_allocators() {
+  using Iterators = types::type_list<
+    cpp20_input_iterator<T*>,
+    forward_iterator<T*>,
+    bidirectional_iterator<T*>,
+    random_access_iterator<T*>,
----------------



================
Comment at: libcxx/test/support/from_range_sequence_containers.h:33
+  T array[] = {
+    make<T>(0), make<T>(1), make<T>(2), make<T>(3), make<T>(4), make<T>(5), make<T>(6), make<T>(7), make<T>(8)
+  };
----------------
Let's test on an empty array, a 1-sized array, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142335