[libcxx-commits] [PATCH] D128416: [libc++] Implement the deque functions of P1206R7

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this. I did a shallow review and hope to find time for a more in depth review at another time.



================
Comment at: libcxx/include/CMakeLists.txt:195
   __concepts/constructible.h
+  __concepts/container_compatible_range.h
   __concepts/convertible_to.h
----------------
Can you update the C++2b status page with the status `|Partial|` and add a note which section is implemented.
(I guess a full status page is a bit overkill.)


================
Comment at: libcxx/include/__concepts/container_compatible_range.h:24
+
+template <class _Range, class _Type>
+concept __container_compatible_range =
----------------
s/_Type/_Tp


================
Comment at: libcxx/include/__ranges/from_range_t.h:22
+
+struct from_range_t {
+  explicit from_range_t() = default;
----------------
Please update the appropriate synopsis.


================
Comment at: libcxx/include/__ranges/from_range_t.h:24
+  explicit from_range_t() = default;
+};
+
----------------
I don't see any tests for this trivial public type.


================
Comment at: libcxx/include/deque:14
 /*
     deque synopsis
 
----------------
Please update the synopsis.


================
Comment at: libcxx/include/deque:1391
+    while (__first1 != __last1 && __first2 != __last2) {
+      *__first2 = *__first1;
+      ++__first1;
----------------
When the range is a temporary are we allowed to move the elements from the range instead?


================
Comment at: libcxx/test/std/containers/sequences/deque/deque.cons/from_range.pass.cpp:15
+
+#include <algorithm>
+#include <array>
----------------
Can we add some test validating that ranges not passing the concept `__container_compatible_range` are rejected? For example an output range?


================
Comment at: libcxx/test/std/containers/sequences/deque/deque.cons/from_range.pass.cpp:47
+bool test() {
+  test_iterators<cpp17_input_iterator<int*>, sentinel_wrapper<cpp17_input_iterator<int*>>>();
+  test_iterators<cpp20_input_iterator<int*>, sentinel_wrapper<cpp20_input_iterator<int*>>>();
----------------
http://eel.is/c++draft/sequence.reqmts#12 requires Cpp17MoveInsertable, please validates this works. (Based on earlier observations I expect it to fail).


================
Comment at: libcxx/test/std/containers/sequences/deque/deque.cons/from_range.pass.cpp:48
+  test_iterators<cpp17_input_iterator<int*>, sentinel_wrapper<cpp17_input_iterator<int*>>>();
+  test_iterators<cpp20_input_iterator<int*>, sentinel_wrapper<cpp20_input_iterator<int*>>>();
+  test_iterators<forward_iterator<int*>>();
----------------
Can you also test the complexity requirements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128416