[libcxx-commits] [PATCH] D149830: [libc++][ranges] Implement the changes to node-based containers from P1206 (`ranges::to`):

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Thanks! I don't have a lot of comments, the patch is quite large but there's a lot of mechanical changes that don't differ from class to class, which makes this easier to review.



================
Comment at: libcxx/include/list:786-788
+      for (auto&& __e : __range) {
+        __emplace_back(std::forward<decltype(__e)>(__e));
+      }
----------------
Maybe we can use `prepend_range(...)` here instead? It would make it the same as what we do for `forward_list`.


================
Comment at: libcxx/include/list:920
+    void append_range(_Range&& __range) {
+      insert_range(end(), std::forward<_Range>(__range));
+    }
----------------
IDK if you should be using `insert(...)` here. However, either all of these new methods should lower to `insert(...)`, or none of them should (and then they should probably be for loops with `emplace_foo(...)`).


================
Comment at: libcxx/include/map:1125
+      : __tree_(__vc(__comp), typename __base::allocator_type(__a)) {
+      insert_range(std::forward<_Range>(__range));
+    }
----------------
`__tree` also has `__assign_unique(_ForwardIterator, _ForwardIterator);` available, I'm wondering whether that should be used instead. I think we have some benchmarks for `std::map`, it would be worth adding some simple benchmark for the `from_range` constructor and checking which approach is fastest.


================
Comment at: libcxx/include/unordered_map:1643
+#if _LIBCPP_STD_VER >= 23
+template <ranges::input_range _Range,
+          class _Hash = hash<__range_key_type<_Range>>,
----------------
Can you validate whether you tested these `enable_if`s in your deduction guide test? I think you want to add that stuff to `AssociativeContainerDeductionGuidesSfinaeAway`.


================
Comment at: libcxx/test/std/containers/associative/from_range_associative_containers.h:27
+template <class Container, class Range>
+concept HasFromRangeCtr = requires (Range&& range) {
+  Container(std::from_range, std::forward<Range>(range));
----------------
This concept doesn't seem to cover ctors of the form `map(from_range_t, R&& rg, const Allocator& a))`. Only those that specify the comparator.


================
Comment at: libcxx/test/std/containers/associative/from_range_associative_containers.h:171
+template <class Container, class Range>
+concept SetHasFromRangeCtr = requires (Range&& range) {
+  Container(std::from_range, std::forward<Range>(range));
----------------
Same comment as above.


================
Comment at: libcxx/test/std/containers/associative/multiset/multiset.cons/from_range.pass.cpp:18
+
+// template<container-compatible-range<value_type> R>
+//   multiset(from_range_t, R&& rg, const Compare& comp = Compare(), const Allocator& = Allocator()); // C++23
----------------
Uber-nitpick but we generally try to have the synopsis comment above the includes. Applies here and elsewhere.


================
Comment at: libcxx/test/std/containers/insert_range_maps_sets.h:253
+
+  { // Full container.
+    // full_c.insert_range(empty_range)
----------------
Or something similar? Full has a connotation that we don't want to have here. It kinda implies "no more capacity", to steal your words. `FullContainer` & friends should also change accordingly.


================
Comment at: libcxx/test/std/containers/unord/unord.set/unord.set.cnstr/deduct.pass.cpp:260-264
+      // FIXME -- the constructor doesn't seem to exist.
+      { // (from_range, range, alloc)
+        //std::unordered_set c(std::from_range, Range(), Alloc());
+        //static_assert(std::is_same_v<decltype(c), std::unordered_set<int, DefaultHash, DefaultPred, Alloc>>);
+      }
----------------
If this is true, this would need a LWG issue. We can add it as a comment here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149830