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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

Can you address outstanding feedback on this review and then split it up as:

- `vector`
- `deque`
- `string`
- node-based containers
- container adaptors
- `ranges::to` (that could be this patch)

I'd like to do the final review on something more focused to make sure we don't miss anything, and also it'll allow staging the check-ins to avoid having to revert 11kLOC in case of issues.



================
Comment at: libcxx/include/forward_list:738
+        const allocator_type& __a = allocator_type()) : base(__a) {
+      insert_range_after(cbefore_begin(), std::forward<_Range>(__range));
+    }
----------------



================
Comment at: libcxx/include/list:912
+      for (auto&& __e : __range) {
+        __emplace_back(std::forward<decltype(__e)>(__e));
+      }
----------------
Can you remove `std::forward` here and confirm that the tests fail? Edit: Confirmed it fails.


================
Comment at: libcxx/include/list:1556
 {
     _LIBCPP_DEBUG_ASSERT(__get_const_db()->__find_c_from_i(_VSTD::addressof(__p)) == this,
                          "list::insert(iterator, range) called with an iterator not referring to this list");
----------------
It feels to me like this check should be in `__insert_with_sentinel`?


================
Comment at: libcxx/include/string:1326
+    constexpr basic_string& append_range(_Range&& __range) {
+      return append(basic_string(from_range, std::forward<_Range>(__range), get_allocator()));
+    }
----------------
This one can be more efficient. If we have an input range, then we do need to make a temporary string and append it. But if we have e.g. a forward_range or a sized_range, we don't need to make a copy of the input range, and I think we should really avoid doing it. I think simply using `insert_range(end(), ...)` here would solve that problem?


================
Comment at: libcxx/include/string:1973
+    template <class _InputIterator, class _Sentinel>
+    _LIBCPP_CONSTEXPR_SINCE_CXX20
+    void __init_with_size(_InputIterator __first, _Sentinel __last, size_type __sz) {
----------------
Let's try to reduce the diff, this seems like it's only moved around.


================
Comment at: libcxx/include/vector:1470
+template <class _Tp, class _Allocator>
+template <class _Iterator, class _Sentinel>
+_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI
----------------
You need to retain the fact that this is at least a forward iterator, otherwise this method doesn't work properly.


================
Comment at: libcxx/include/vector:1478-1479
         {
-            __growing = true;
-            __mid =  __first;
-            std::advance(__mid, size());
+            std::advance(__first, size());
+            __construct_at_end(__first, __last, __new_size - size());
+
----------------
This doesn't seem to work, I think we're consuming the first `size()` elements from `__first` and then copying the rest into the vector only. I think this also means there's a gap in the testing.


================
Comment at: libcxx/include/vector:1921
 {
-    _LIBCPP_DEBUG_ASSERT(__get_const_db()->__find_c_from_i(std::addressof(__position)) == this,
-                         "vector::insert(iterator, range) called with an iterator not referring to this vector");
+  _LIBCPP_DEBUG_ASSERT(__get_const_db()->__find_c_from_i(std::addressof(__position)) == this,
+                        "vector::insert(iterator, range) called with an iterator not referring to this vector");
----------------
This should be in `__insert_with_sentinel`, I think.


================
Comment at: libcxx/include/vector:1975
 {
-    _LIBCPP_DEBUG_ASSERT(__get_const_db()->__find_c_from_i(std::addressof(__position)) == this,
-                         "vector::insert(iterator, range) called with an iterator not referring to this vector");
+  _LIBCPP_DEBUG_ASSERT(__get_const_db()->__find_c_from_i(std::addressof(__position)) == this,
+                        "vector::insert(iterator, range) called with an iterator not referring to this vector");
----------------
Same, should be in `__insert_with_size`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142335