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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

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

Thanks, this looks good, except for the style not matching the style of the surrounding code.



================
Comment at: libcxx/include/queue:57
+    template<class InputIterator>
+    queue(InputIterator first, InputIterator last);               // since C++23
 
----------------
Nits: please indent to match the surrounding style, and please `last); // since C++23` (although I see you're aligning the comments, I don't think it's beneficial here).

Also, please shuffle the order of these new constructors to match what's in the paper and/or Standard synopsis. The `(first, last)` one should be circa line 44, and the `(first, last, const Alloc&)` one down here.


================
Comment at: libcxx/include/queue:84-86
+template<class InputIterator, class Allocator>
+queue(InputIterator, InputIterator, Allocator)
+-> queue<iter-value-type<InputIterator>, deque<iter-value-type<InputIterator>, Allocator>>; // since C++23
----------------
Here and line 79 also, please indent the second line by 2 spaces to match the surrounding style.
Consider line-breaking before `deque` just to improve readability.


================
Comment at: libcxx/include/stack:199
+    template <class _InputIterator>
+    _LIBCPP_HIDE_FROM_ABI stack(_InputIterator __first, _InputIterator __last) : c{__first, __last} {}
+
----------------
Ditto below.


================
Comment at: libcxx/test/std/containers/container.adaptors/queue/queue.cons.alloc/ctor_iterators.pass.cpp:30-31
+public:
+  GetAlloc(test_allocator_statistics& stats_, iter begin, iter end)
+      : base_type{begin, end, test_allocator<int>{&stats_}}, stats{&stats_} {}
+  void check() {
----------------



================
Comment at: libcxx/test/std/containers/container.adaptors/queue/queue.cons/ctor_iterators.pass.cpp:24
+  const auto a = std::array{4, 3, 2, 1};
+  auto queue = std::queue{a.begin(), a.end()};
+  assert(queue.front() == 4);
----------------
Testing that `std::queue{it, it}` produces a queue of ints rather than a queue of iterators is a //good// test, but it belongs in `deduct.pass.cpp`, not here. In this file, you should focus on the non-CTAD happy path, `std::queue<int>(a.begin(), a.end())`. Keep it simple:
```
const int a[] = {4, 3, 2, 1};
std::queue<int> queue(a, a+4);
assert(queue.front() == 4); // etc.
```


================
Comment at: libcxx/test/std/containers/container.adaptors/queue/queue.cons/deduct.pass.cpp:159-164
+    {
+      std::array<int, 0> a;
+      ASSERT_SAME_TYPE(decltype(std::queue{a.begin(), a.end()}), std::queue<int>);
+      ASSERT_SAME_TYPE(decltype(std::queue{a.begin(), a.end(), test_allocator<int>{}}),
+                       std::queue<int, std::deque<int, test_allocator<int>>>);
+    }
----------------
Let's use `std::list<int>` as our type here, since we do that above. Also, let's match the style of surrounding code.
```
{
    typedef short T;
    typedef test_allocator<T> Alloc;
    std::list<T> source;
    {
    std::queue que(source.begin(), source.end());
    static_assert(std::is_same_v<decltype(que), std::queue<T>>);
    }
    {
    std::queue que(source.begin(), source.end(), Alloc(2));
    static_assert(std::is_same_v<decltype(que), std::queue<T, std::deque<T, Alloc>>>);
    }
}
```


================
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>{}}),
----------------
Mordante wrote:
> I miss a test to validate the queue has the expected values.
I miss variable names that make it clear that the thing under test here is not a queue. ;)
(This test should be refactored simpler along the same lines I suggest for the queue test, above.)


================
Comment at: libcxx/test/std/containers/container.adaptors/stack/stack.cons/deduct.pass.cpp:163-168
+    {
+      std::array<int, 0> a;
+      ASSERT_SAME_TYPE(decltype(std::stack{a.begin(), a.end()}), std::stack<int>);
+      ASSERT_SAME_TYPE(decltype(std::stack{a.begin(), a.end(), test_allocator<int>{}}),
+                       std::stack<int, std::deque<int, test_allocator<int>>>);
+    }
----------------
Same comments here as on the queue `deduct.pass.cpp`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115977