[libcxx-commits] [PATCH] D131898: [libc++] Implement P0591R4 (Utility functions to implement uses-allocator construction)

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__memory/uses_allocator_construction.h:22
+
+#if _LIBCPP_STD_VER > 14
+
----------------
huixie90 wrote:
> I am slightly confused by the versions. The paper seems to be C++20, but all the internal `__` functions are targeting C++17, and then the tests are running on all language versions?
`polymorphic_allocator` uses uses-allocator-construction, which this implements. So instead of implementing it twice we can use the same implementation. The tests were just an oversight.


================
Comment at: libcxx/include/__memory/uses_allocator_construction.h:30
+
+template <
+    class _Type,
----------------
ldionne wrote:
> Maybe this should be three functions instead, it would be easier to read.
I read the standard again, and the program is actually ill-formed if none of the `if constexpr` conditions are met.


================
Comment at: libcxx/include/__memory/uses_allocator_construction.h:51
+
+template <class _Pair, class _Alloc, class _Tuple1, class _Tuple2, class = __enable_if_t<__is_std_pair<_Pair>>>
+_LIBCPP_HIDE_FROM_ABI constexpr auto
----------------
huixie90 wrote:
> As you mentioned in another thread, should be use `int = __enable_if_t<b, int> = 0` instead through out the change?
Yes. I wrote this before the discussion. Thanks for not letting me produce more work for myself!


================
Comment at: libcxx/include/__memory/uses_allocator_construction.h:53
+_LIBCPP_HIDE_FROM_ABI constexpr auto
+__uses_allocator_construction_args(const _Alloc& __alloc, piecewise_construct_t, _Tuple1&& __x, _Tuple2&& __y) {
+  return std::make_tuple(
----------------
huixie90 wrote:
> is `noexcept` missing here? Is there a test?
Good catch! Added a regression test.


================
Comment at: libcxx/include/__memory/uses_allocator_construction.h:151
+  struct __pair_constructor {
+    using _PairMut = remove_cv_t<_Pair>;
+
----------------
huixie90 wrote:
> nit: what does `Mut` mean?
mutable


================
Comment at: libcxx/include/__memory/uses_allocator_construction.h:167
+
+  return std::make_tuple(__pair_constructor(__alloc, __value));
+}
----------------
huixie90 wrote:
> does parenthesis work for c++17 to init aggregates?
Nope, replaced it with `{}`.


================
Comment at: libcxx/test/std/utilities/memory/allocator.uses/allocator.uses.construction/uses_allocator_construction_args.pass.cpp:62
+  {
+    [[maybe_unused]] std::same_as<std::tuple<std::allocator_arg_t, const Alloc&>> auto ret =
+        std::uses_allocator_construction_args<UsesAllocArgT>(a);
----------------
huixie90 wrote:
> why `maybe_unused`? it seems that you are using the result in most of the tests
Because I forgot to remove it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131898