[libcxx-commits] [PATCH] D149827: [libc++][ranges] Implement the changes to `deque` from P1206 (`ranges::to`):

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM w/ comments. In particular it would be nice to confirm that this doesn't introduce a regression in the existing deque methods (I don't think it would, but it's simple to confirm). Thanks!



================
Comment at: libcxx/include/deque:1293-1304
+void deque<_Tp, _Allocator>::__assign_with_size(_Iterator __f, difference_type __n) {
+    if (static_cast<size_type>(__n) > size())
+    {
+        auto __i = begin();
+        while (__i != end()) {
+          *__i++ = *__f++;
+        }
----------------
Feel free to `clang-format` this bit.


================
Comment at: libcxx/include/deque:1297
+        auto __i = begin();
+        while (__i != end()) {
+          *__i++ = *__f++;
----------------
I would either use the integer as a counter (since then we know comparison is cheap), or at least cache `end()` since it's kind of expensive and we don't know for sure whether the compiler is smart enough hoist it out.


================
Comment at: libcxx/test/std/containers/sequences/deque/deque.cons/from_range.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
I think we should also add benchmarks in `libcxx/benchmarks/ContainerBenchmarks.h` for the new construction methods we're adding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149827