[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 inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/queue:277
+    template <class _InputIterator, class _Alloc>
+    _LIBCPP_HIDE_FROM_ABI queue(_InputIterator __first, _InputIterator __second, const _Alloc& __alloc)
+        : c(__first, __second, __alloc) {}
----------------
Does this overload need `, __enable_if_t<uses_allocator<container_type, _Alloc>::value>* = 0` similar to the below overloads? I suspect it does. (And then please add a regression test. The regression test will use SFINAE but not CTAD.)


================
Comment at: libcxx/include/queue:397-404
+#if _LIBCPP_STD_VER > 20
+template <class _InputIterator>
+queue(_InputIterator, _InputIterator) -> queue<__iter_value_type<_InputIterator>>;
+
+template <class _InputIterator, class _Alloc>
+queue(_InputIterator, _InputIterator, _Alloc)
+  -> queue<__iter_value_type<_InputIterator>, deque<__iter_value_type<_InputIterator>, _Alloc>>;
----------------
Quuxplusone wrote:
> Yipes, almost missed this, except that the failing CI alerted me. All these deduction guides need to use `__enable_if_t` to disable themselves when `_InputIterator` is not an iterator or `_Alloc` is not an allocator. Look at the guides for `deque` or `priority_queue` and do as they do.
> 
> Please also add some `SFINAES_away` tests to `deduct.pass.cpp` proving that the deduction guides indeed SFINAE away in those cases.
Please follow the pattern on lines 385–397 above (and in e.g. `<vector>`, which I assume is more or less where you copied these particular constraints from):
```
template<class _InputIterator,
         class = enable_if_t<__is_cpp17_input_iterator<_InputIterator>::value>
>
queue(_InputIterator, _InputIterator)
    -> queue<__iter_value_type<_InputIterator>>;

template<class _InputIterator,
         class _Alloc,
         class = enable_if_t<__is_cpp17_input_iterator<_InputIterator>::value>,
         class = enable_if_t<__is_allocator<_Alloc>::value>
>
queue(_InputIterator, _InputIterator, _Alloc)
    -> queue<__iter_value_type<_InputIterator>, deque<__iter_value_type<_InputIterator>, _Alloc>>;
```
Both for consistency, and because `_LIBCPP_HAS_NO_CONCEPTS` is still a thing.


================
Comment at: libcxx/test/std/containers/container.adaptors/stack/stack.cons.alloc/ctor_iterators.pass.cpp:23
+using base_type = std::stack<int, std::deque<int, test_allocator<int>>>;
+using iter = std::array<int, 4>::const_iterator;
+
----------------
`iter` should now be just `const int*`, which means you don't need the typedef.


================
Comment at: libcxx/test/std/containers/container.adaptors/stack/stack.cons/ctor_iterators.pass.cpp:27-30
+  for (const auto& e : a | std::views::reverse) {
+    assert(e == stack.top());
+    stack.pop();
+  }
----------------
This is //okay//, but personally I'd //rather// see lines 25,27,28,29,30 replaced with
```
assert(stack.top() == 1); stack.pop();
assert(stack.top() == 2); stack.pop();
assert(stack.top() == 3); stack.pop();
assert(stack.top() == 4); stack.pop();
assert(stack.empty());
```
and then you can remove the include of `<ranges>`. And similarly in the queue test:
```
assert(queue.front() == 4); queue.pop();
assert(queue.front() == 3); queue.pop();
assert(queue.front() == 2); queue.pop();
assert(queue.front() == 1); queue.pop();
assert(queue.empty());
```


================
Comment at: libcxx/test/std/containers/container.adaptors/stack/stack.cons/deduct.pass.cpp:19
 
+#include <array>
 #include <stack>
----------------
This can be removed again now. Ditto in `queue.cons/ctor_iterators.pass.cpp`.


================
Comment at: libcxx/test/std/containers/container.adaptors/stack/stack.cons/deduct.pass.cpp:159
+#if TEST_STD_VER > 20
+        static_assert(SFINAEs_away<std::stack, Iter, NotIter>);
+        static_assert(SFINAEs_away<std::stack, Iter, NotIter, Alloc>);
----------------
Throw in `<std::stack, NotIter, Iter>` while you're here.
I like the renaming of `BadAlloc` to `NotAlloc`. I wouldn't complain if you replaced the uses of `AllocAsCont` with just `Alloc`, either (I don't think the `AsCont` improves my understanding of that case).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115977