[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.

In D115977#3223823 <https://reviews.llvm.org/D115977#3223823>, @jloser wrote:

> I haven't finished reviewing this in full yet, but do we need to use `enable_if_t` instead of concepts/`requires` since this is C++23 code? I.e. is our requirement of using older `AppleClang` version in C++23 mode an issue with this (and therefore we must use `enable_if_t` still)? The consistency argument is fine - I'm asking purely from a technical standpoint.

If you want it to be present in `-std=c++2b` mode, on old compilers without full concepts support, then it can't depend on full concepts support (although technically maybe those old compilers support //enough// of concepts that a simple `requires`-clause would be OK... but let's not go down that rabbit hole, let's just wait a couple more years). If you guard it under `_LIBCPP_HAS_NO_CONCEPTS`, then it //won't be present// on old compilers without full concepts support, so I don't think that would be a good idea either. Finally, you could provide two different implementations — one under `_LIBCPP_HAS_NO_CONCEPTS` and one not — but that would be twice the work, and require going down a rabbit hole involving "twice the tests," so let's not do that either.



================
Comment at: libcxx/include/queue:228
 #include <functional>
+#include <memory>
+#include <type_traits>
----------------
Quuxplusone wrote:
> Why all of `<memory>`? (Same question below in `stack`.)
`stack` still includes `<memory>`; it probably shouldn't, right?


================
Comment at: libcxx/include/queue:85
+template<class InputIterator, class Allocator>
+queue(InputIterator, InputIterator, Allocator)
+  -> queue<iter-value-type<InputIterator>,
----------------



================
Comment at: libcxx/include/queue:282
+              class = __enable_if_t<__is_cpp17_input_iterator<_InputIterator>::value>,
+              class = __enable_if_t<__is_allocator<_Alloc>::value>,
+              class = __enable_if_t<uses_allocator<container_type, _Alloc>::value>>
----------------
This line shouldn't be here; and please add a regression test if possible.
An example of a regression test would be
```
int a[10] = {};
std::pmr::queue<int> q(a, a+10, std::pmr::new_delete_resource());  // should be OK
```
except that that'll be hard to put in a test since we don't support std::pmr. :P

The //deduction guides// are appropriately constrained so that (if they don't take a `Container` parameter) `Alloc` must be an allocator; but the //constructors// are not so constrained. https://wg21.link/p1518r1 is related.


================
Comment at: libcxx/include/stack:210
+              class = __enable_if_t<__is_cpp17_input_iterator<_InputIterator>::value>,
+              class = __enable_if_t<__is_allocator<_Alloc>::value>,
+              class = __enable_if_t<uses_allocator<container_type, _Alloc>::value>>
----------------
and regression-test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115977