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

[Hui via Phabricator via libcxx-commits]

huixie90 requested changes to this revision.
huixie90 added inline comments.


================
Comment at: libcxx/include/__memory/uses_allocator_construction.h:22
+
+#if _LIBCPP_STD_VER > 14
+
----------------
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?


================
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
----------------
As you mentioned in another thread, should be use `int = __enable_if_t<b, int> = 0` instead through out the change?


================
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(
----------------
is `noexcept` missing here? Is there a test?


================
Comment at: libcxx/include/__memory/uses_allocator_construction.h:87
+_LIBCPP_HIDE_FROM_ABI constexpr auto
+__uses_allocator_construction_args(const _Alloc& __alloc, pair<_Up, _Vp>& __pair) noexcept {
+  return std::__uses_allocator_construction_args<_Pair>(
----------------
I believe this is part of the zip paper, which is c++23. perhaps you need to branch under different language versions?


================
Comment at: libcxx/include/__memory/uses_allocator_construction.h:111
+_LIBCPP_HIDE_FROM_ABI constexpr auto
+__uses_allocator_construction_args(const _Alloc& __alloc, const pair<_Up, _Vp>&& __pair) noexcept {
+  return std::__uses_allocator_construction_args<_Pair>(
----------------
I believe this is part of the "zip" paper and perhaps you can update the zip project csv as well. I have a patch related to `pair` ongoing as well which skipped this part
https://reviews.llvm.org/D131495


================
Comment at: libcxx/include/__memory/uses_allocator_construction.h:122
+template <class _Ap, class _Bp>
+void __fun(const pair<_Ap, _Bp>&);
+
----------------
nit: can we change its name to be more unique? the chance of other people using the name `__detail::__fun` is not zero


================
Comment at: libcxx/include/__memory/uses_allocator_construction.h:131
+template <class _Tp>
+constexpr bool __convertible_to_const_pair_ref =
+    decltype(__detail::__convertible_to_const_pair_ref_impl<_Tp>(0))::value;
----------------
`inline`


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


================
Comment at: libcxx/include/__memory/uses_allocator_construction.h:167
+
+  return std::make_tuple(__pair_constructor(__alloc, __value));
+}
----------------
does parenthesis work for c++17 to init aggregates?


================
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);
----------------
why `maybe_unused`? it seems that you are using the result in most of the tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131898