[libcxx-commits] [PATCH] D115977: [libc++] Implement P1425R4 (Iterator pair constructors for std::stack and std::queue)

[Mark de Wever via Phabricator via libcxx-commits]

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

Thanks for your contribution! It still needs some more work.



================
Comment at: libcxx/include/queue:77
   queue(Container) -> queue<typename Container::value_type, Container>; // C++17
 
 template<class Container, class Allocator>
----------------
This deduction guide has also been added. A quick scan it seems there are more deduction guides not added to the synopsis.


================
Comment at: libcxx/include/queue:261
     queue(const queue& __q) : c(__q.c) {}
 
     _LIBCPP_INLINE_VISIBILITY
----------------
Can you move the new constructors to this location, that seems more consistent.


================
Comment at: libcxx/test/std/containers/container.adaptors/queue/queue.cons.alloc/ctor_iterators.pass.cpp:41
+  test_allocator_statistics stats{};
+  const auto queue = GetAlloc{stats, a.begin(), a.end()};
+  ASSERT_SAME_TYPE(decltype(std::queue{a.begin(), a.end(), test_allocator<int>{}}),
----------------
I miss a test to validate the queue has the expected values.


================
Comment at: libcxx/test/std/containers/container.adaptors/queue/queue.cons.alloc/ctor_iterators.pass.cpp:43
+  ASSERT_SAME_TYPE(decltype(std::queue{a.begin(), a.end(), test_allocator<int>{}}),
+                   std::queue<int, std::deque<int, test_allocator<int>>>);
+}
----------------
The deduction guides should be their own test, `queue.cons/deduct.pass.cpp` already contains test for the deduction guides, please add them there.


================
Comment at: libcxx/test/std/containers/container.adaptors/queue/queue.cons/ctor_iterators.pass.cpp:28
+  assert(queue.back() == 1);
+  assert(queue.size() == 4);
+}
----------------
I think it would be good to test all elements in the queue to have the expected value.


================
Comment at: libcxx/test/std/containers/container.adaptors/stack/stack.cons.alloc/ctor_iterators.pass.cpp:41
+  test_allocator_statistics stats{};
+  const auto queue = GetAlloc{stats, a.begin(), a.end()};
+  ASSERT_SAME_TYPE(decltype(std::stack{a.begin(), a.end(), test_allocator<int>{}}),
----------------
I miss a test to validate the queue has the expected values.


================
Comment at: libcxx/test/std/containers/container.adaptors/stack/stack.cons/ctor_iterators.pass.cpp:27
+  assert(queue.top() == 1);
+  assert(queue.size() == 4);
+}
----------------
I think it would be good to test all elements in the queue to have the expected value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115977