Implement uses-allocator (P0591R4)

[ldionne]

Summary

Add uses_allocator_construction_args overloads, make_obj_using_allocator and uninitialized_construct_using_allocator.

[zoecarver]

The paper also requests the deletion of several construct methods. I did not add this to the patch because it broke a lot of tests (and other things). I thought it would be worth asking if we should keep them for compatibility reasons.

[zoecarver]

Check off in status

[zoecarver]

Fix trailing white space.

[zoecarver]

• Completely re-write this patch.
• Add tests
• Fix tuple issues

[zoecarver]

Because of the length of the name uses_allocator_construction_args, some of the styles are... different. Feel free to suggest changes.

include/memory

5813

Spacing

include/tuple

1377 ↗

(On Diff #197159)

I can submit a separate patch for this.

test/std/utilities/memory/allocator.uses/allocator.uses.construction/uses_allocator_construction_args.pass.cpp

9 ↗

(On Diff #197159)

Add synopsis comment

[zoecarver]

• Finishes implementing the paper (remove scoped_allocator::construct)
• Updates and adds tests

I think P0475R1 can also be marked complete because the code it refers to has been removed.

[zoecarver]

• finally finish implementing the paper
• remove construct methods from polymorphic_allocator

[zoecarver]

include/experimental/memory_resource

203 ↗

(On Diff #198213)

Please note: this is not tested (and probably needs further changes). Because of 41760 (which I am starting to think is my fault and not an actual bug), I was unable to run many of the polymorphic_allocator tests and therefore could not ensure that this was correctly implemented.

include/tuple

1377 ↗

(On Diff #197159)

Given that a fix for this goes against the standard, I am not sure what to do here. First, I am going to look into this more and make sure it is necessary.

As a note, the change here is not my suggested fix. Please see D61293 for that.

[• Quuxplusone]

include/memory

5854

What is -> auto doing here and on line 5755?

5901

So if I pass in a pair<string, int> or pair<string&&, int>, you'll move out of p.first, but if I pass in pair<string&, int>, you'll make a copy of p.first, is that right? I guess that seems reasonable, but it's a surprising/smelly use of std::forward IMVHO.

include/scoped_allocator

515 ↗

(On Diff #198213)

This function body is indented an extra level. Not your patch's fault, but I wonder if your patch should fix it.

test/std/utilities/allocator.adaptor/allocator.adaptor.members/construct_pair.pass.cpp

49 ↗

(On Diff #198213)

What's the rationale for all these test changes? I strongly suspect that they are a bad idea: if the original tests fail after your patch, I think there must be something wrong with your patch.

[zoecarver]

include/memory

5854

Will fix.

5901

You are right; I should probably use std::move here. For reference here is what the standard says.

include/scoped_allocator

515 ↗

(On Diff #198213)

Yep, good catch. I can fix that.

test/std/utilities/allocator.adaptor/allocator.adaptor.members/construct_pair.pass.cpp

49 ↗

(On Diff #198213)

These are all simply adding const or changing the reference type (I am not saying that necessarily makes them okay though). Because of how uses_allocator_construction_args is defined in the standard, make_tuple is passed an rvalue piecewise_construct (that is why the first part of this changed). I think the others are similar.

I will take a closer look at this and see which test is correct.

[• Quuxplusone]

include/memory

5901

Thanks for pointing to the relevant Standard wording! I was too lazy to go look for it myself. :) The wording uses std::move(pr).first, which is equivalent to std::forward<_T1>(pr.first) but doesn't set off my red-flag detectors — I would greatly prefer _VSTD::move(__p).first here.

(Notice that _VSTD::move(__p).first is different from _VSTD::move(__p.first) when __p is a pair of lvalue references. Which is good; it's what we want.)

[zoecarver]

• disable experimental tests
• update other tests

I removed the temporary "fix" I added to make_from_tuple because it was causing other failures and I wanted to discuss it before implementing something. The issue has to do with the move vs forward thread below. Using std::move(pair).first changes rvalues into lvalues and vice versa whereas forward consistently makes everything an rvalue (in the previous implementation, obviously not always). This clearly creates issues and is the reason that the tests are failing.

[zoecarver]

test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_pair_const_lvalue_pair.pass.cpp

122 ↗

(On Diff #199196)

I really don't like disabling tests, but I think that these tests are incorrect (at least for c++20). Uses-allocator construction requires that types (or pair types) implement a uses-allocator typedef that is the same as the allocator they are being passed. This means that when these tests are passed the wrong allocator type, they choose the incorrect (well... actually it is correct) overload (the first one). If I am mistaken about this, let me know, and I will change the uses-allocator implementation.

[zoecarver]

test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_pair_const_lvalue_pair.pass.cpp

122 ↗

(On Diff #199196)

I think 2975 might fix this actually.

[zoecarver]

test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_pair_const_lvalue_pair.pass.cpp

122 ↗

(On Diff #199196)

This is an experimental library! The patch should not touch it.

[• Quuxplusone]

include/memory

5923

Nit: I believe this needs a _VSTD:: to prevent ADL in some complicated case involving std::pair<int, std::vector<Incomplete>*> that I don't 100% remember. (Something like it's going to try to look for friends of Incomplete because Incomplete is an associated entity of that pair type.) It would be interesting to write a unit test for this.
Rule of thumb, every function call needs a _VSTD:: to prevent ADL for _some_ reason or other. :)

[zoecarver]

As I have worked through fixing all the issues around this patch, I think I have come to the two root problems. First, using forward_as_tuple makes it so that the tuple cannot be copied (rightfully so) and are therefore useless in make_from_tuple. Here is an example. This should probably be fixed with an LWG issue (either to change what tuples can be copied or update make_from_tuple). Second, and more important, the expression forward_as_tuple(move(p).second) is unable to preserve rvalueness of a type. Here is an example. I think the paper either needs to be updated or an issue needs to be presented to fix this (I am happy to write that issue). What are your thoughts @mclow.lists, @EricWF, and @Quuxplusone?

[zoecarver]

include/memory

5901

I am not sure that is necessarily true. Sometimes one works, and the other doesn't. I think it should be pointed out that the author of the paper used forward in their implementation (probably for that reason).

[zoecarver]

include/memory

5923

Do you mean __uses_allocator_construction_args_impl or is_pair?

[mclow.lists]

Please make sure that you implement LWG 3185 as part of this patch

[zoecarver]

In D58879#1546317, @mclow.lists wrote:

Please make sure that you implement LWG 3185 as part of this patch

Will do. Thanks.

[MaskRay]

include/memory

5943

Should make_obj_using_allocator be qualified?
https://github.com/cplusplus/draft/issues/3111
https://github.com/gcc-mirror/gcc/commit/0c3543005cee13158806fe2909de9920522d2f9e

[ldionne]

[Github PR transition cleanup]

This was done in D131898.

include/memory

template<class T>
     atomic_compare_exchange_weak_explicit(shared_ptr<T>* p, shared_ptr<T>* v,
                                           shared_ptr<T> w, memory_order success,
                                           memory_order failure);
 template<class T>
     bool
     atomic_compare_exchange_strong_explicit(shared_ptr<T>* p, shared_ptr<T>* v,
                                             shared_ptr<T> w, memory_order success,
                                             memory_order failure);
+ 
+template <class T, class Alloc, class... Args>
+  auto uses_allocator_construction_args(const Alloc& alloc, Args&&... args);
+template <class T, class Alloc, class Tuple1, class Tuple2>
+  auto uses_allocator_construction_args(const Alloc& alloc, piecewise_construct_t, Tuple1&& x, Tuple2&& y);
+template <class T>
+  auto uses_allocator_construction_args(const Alloc& alloc);
+template <class T, class Alloc, class U, class V>
+  auto uses_allocator_construction_args(const Alloc& alloc, U&& u, V&& v);
+template <class T, class Alloc, class U, class V>
+  auto uses_allocator_construction_args(const Alloc& alloc, const pair<U,V>& pr);
+template <class T, class Alloc, class U, class V>
+  auto uses_allocator_construction_args(const Alloc& alloc, pair<U,V>&& pr);
+template <class T, class Alloc, class... Args>
+  T make_obj_using_allocator(const Alloc& alloc, Args&&... args);
+template <class T, class Alloc, class... Args>
+  T* uninitialized_construct_using_allocator(T* p, const Alloc& alloc, Args&&... args);
+
 // Hash support
 template <class T> struct hash;
 template <class T, class D> struct hash<unique_ptr<T, D> >;
 template <class T> struct hash<shared_ptr<T> >;
 
 template <class T, class Alloc>
   inline constexpr bool uses_allocator_v = uses_allocator<T, Alloc>::value;
 

 struct __is_allocator : false_type {};
 
 template<typename _Alloc>
 struct __is_allocator<_Alloc,
        typename __void_t<typename _Alloc::value_type>::type,
        typename __void_t<decltype(_VSTD::declval<_Alloc&>().allocate(size_t(0)))>::type
      >
    : true_type {};
     
+#if _LIBCPP_STD_VER > 17
+
+template <class _Tp> struct __is_pair : false_type { };
+
+template <class _T1, class _T2>
+struct __is_pair<pair<_T1, _T2>> : true_type { };
+
+template<class _Tp, class _Un, class _Alloc, class... _Args>
+auto
+__uses_allocator_construction_args_no_pair(false_type, _Un, const _Alloc, _Args&&... __args) {
+  return _VSTD::forward_as_tuple(_VSTD::forward<_Args>(__args)...);
+}
+
+template<class _Tp, class _Alloc, class... _Args>
+auto
+__uses_allocator_construction_args_no_pair(true_type, true_type, const _Alloc __alloc, _Args&&... __args) {
+  return _VSTD::forward_as_tuple(allocator_arg, __alloc, _VSTD::forward<_Args>(__args)...);
+}
+
+template<class _Tp, class _Alloc, class... _Args>
+auto
+ __uses_allocator_construction_args_no_pair(false_type, false_type, const _Alloc __alloc, _Args&&... __args) {
+  return _VSTD::forward_as_tuple(_VSTD::forward<_Args>(__args)..., __alloc);
+}
+
+template<class _Tp, class _Alloc, class... _Args,
+  typename = enable_if_t<!__is_pair<_Tp>::value>>
+auto
+uses_allocator_construction_args(const _Alloc& __alloc, _Args&&... __args) {
+  return __uses_allocator_construction_args_no_pair<_Tp>(
+    _VSTD::uses_allocator<_Tp, _Alloc>(),
+    _VSTD::is_constructible<_Tp, allocator_arg_t, _Alloc, _Args...>(),
+    __alloc, _VSTD::forward<_Args>(__args)...);
+}
+  
+template<class _Tp, class _Alloc, class _T1, class _T2,
+  typename = enable_if_t<__is_pair<_Tp>::value>>
+auto
+uses_allocator_construction_args(const _Alloc& __alloc,
+                 piecewise_construct_t,
+                 _T1&& __a, _T2&& __b) {
+  return std::make_tuple(piecewise_construct,
+               _VSTD::apply( [&__alloc](auto&&... __args) {
+    return _VSTD::uses_allocator_construction_args<_T1>(__alloc, _VSTD::forward<decltype(__args)>(__args)...);
+  }, _VSTD::forward<_T1>(__a)),
+               _VSTD::apply( [&__alloc](auto&&... __args) {
+    return _VSTD::uses_allocator_construction_args<_T2>(__alloc, _VSTD::forward<decltype(__args)>(__args)...);
+  }, _VSTD::forward<_T2>(__b)));
+}
+
+template<class _Tp, class _Alloc,
+  typename = enable_if_t<__is_pair<_Tp>::value>>
+auto
+uses_allocator_construction_args(const _Alloc& __alloc) {
+  return uses_allocator_construction_args<_Tp>(__alloc, piecewise_construct, _VSTD::tuple<>{}, _VSTD::tuple<>{});
+}
+
+template<class _Tp, class _Alloc, class _T1, class _T2,
+  typename = enable_if_t<__is_pair<_Tp>::value>>
+auto
+uses_allocator_construction_args(const _Alloc& __alloc, _T1&& __a, _T2&& __b) {
+  return uses_allocator_construction_args<_Tp>(
+    __alloc, piecewise_construct,
+    _VSTD::forward_as_tuple(_VSTD::forward<_T1>(__a)),
+    _VSTD::forward_as_tuple(_VSTD::forward<_T2>(__b)));
+}
+
+template<class _Tp, class _Alloc, class _T1, class _T2,
+  typename = enable_if_t<__is_pair<_Tp>::value>>
+auto
+uses_allocator_construction_args(const _Alloc& __alloc, const _VSTD::pair<_T1, _T2>& __p) {
+  return uses_allocator_construction_args<_Tp>(__alloc, piecewise_construct,
+    forward_as_tuple(__p.first),
+    forward_as_tuple(__p.second));
+}
+
+template<class _Tp, class _Alloc, class _T1, class _T2,
+  typename = enable_if_t<__is_pair<_Tp>::value>>
+auto
+uses_allocator_construction_args(const _Alloc& __alloc, _VSTD::pair<_T1, _T2>&& __p) {
+  return uses_allocator_construction_args<_Tp>(__alloc,
+    piecewise_construct,
+    forward_as_tuple(_VSTD::move(__p).first),
+    forward_as_tuple(_VSTD::move(__p).first));
+}
+
+template<class _Tp, class _Alloc, class... _Args>
+_Tp
+make_obj_using_allocator(const _Alloc& __alloc, _Args&&... __args) {
+  return make_from_tuple<_Tp>(
+    uses_allocator_construction_args<_Tp>(__alloc, _VSTD::forward<_Args>(__args)...));
+}
+
+template<class _Tp, class _Alloc, class... _Args>
+_Tp*
+uninitialized_construct_using_allocator(_Tp* __p, const _Alloc& __alloc, _Args&&... __args) {
+  return ::new(static_cast<void*>(__p)) _Tp(
+    make_obj_using_allocator<_Tp>(__alloc, _VSTD::forward<_Args>(__args)...)
+  );
+}
+
+#endif // _LIBCPP_STD_VER > 17
+
 _LIBCPP_END_NAMESPACE_STD
 
 _LIBCPP_POP_MACROS
 
 #endif  // _LIBCPP_MEMORY

test/std/utilities/memory/uses_allocator_construction_args.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <memory>
+
+#include <memory>
+#include <utility>
+#include <cassert>
+#include <iostream>
+
+using pair_t = std::pair<int, double>;
+
+struct A
+{
+  int a;
+  float b;
+
+  A(int a, float b): a(a), b(b) { }
+};
+
+template <class... Args>
+void test_pair (Args&&... args)
+{
+  auto args_tuple = std::uses_allocator_construction_args<pair_t>(std::forward<Args>(args)...);
+  auto arg_count = std::tuple_size<decltype(args_tuple)>::value;
+  assert(arg_count == 3);
+}
+
+int main(int, char**)
+{
+  // non pair contruction
+  std::allocator<A> alloc;
+  auto a = std::make_obj_using_allocator<A>(alloc, 10, 5.0);
+
+  assert(a.a == 10);
+  assert(a.b == 5.0);
+  
+  auto* b = std::uninitialized_construct_using_allocator(&a, alloc, 20, 10.0);
+
+  assert(a.a == 20);
+  assert(a.b == 10.0);
+  
+  assert(b->a == 20);
+  assert(b->b == 10.0);
+  
+  assert(&a == b);
+  
+  // pair construction
+  std::allocator<pair_t> p_alloc;
+  pair_t p { 10, 5.0 };
+  
+  test_pair(p_alloc);
+  test_pair(p_alloc, 10, 5.0);
+  test_pair(p_alloc, p);
+
+  return 0;
+}

www/cxx2a_status.html

-->
   <tr><td><a href="https://wg21.link/P1120R0">P1120R0</a></td><td>CWG</td><td>Consistency improvements for &lt;=&gt; and other comparison operators</td><td>Rapperswil</td><td></td><td></td></tr>
 
    <tr><td></td><td></td><td></td><td></td><td></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0318R1">P0318R1</a></td><td>LWG</td><td>unwrap_ref_decay and unwrap_reference</td><td>San Diego</td><td>Complete</td><td>8.0</td></tr>
   <tr><td><a href="https://wg21.link/P0356R5">P0356R5</a></td><td>LWG</td><td>Simplified partial function application</td><td>San Diego</td><td><i> </i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0357R3">P0357R3</a></td><td>LWG</td><td>reference_wrapper for incomplete types</td><td>San Diego</td><td><i> </i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0482R6">P0482R6</a></td><td>CWG</td><td>char8_t: A type for UTF-8 characters and strings</td><td>San Diego</td><td><i> </i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0487R1">P0487R1</a></td><td>LWG</td><td>Fixing operator&gt;&gt;(basic_istream&amp;, CharT*) (LWG 2499)</td><td>San Diego</td><td>Complete</td><td>8.0</td></tr>
-  <tr><td><a href="https://wg21.link/P0591R4">P0591R4</a></td><td>LWG</td><td>Utility functions to implement uses-allocator construction</td><td>San Diego</td><td><i> </i></td><td></td></tr>
+  <tr><td><a href="https://wg21.link/P0591R4">P0591R4</a></td><td>LWG</td><td>Utility functions to implement uses-allocator construction</td><td>San Diego</td><td>Complete</td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0595R2">P0595R2</a></td><td>CWG</td><td>P0595R2 std::is_constant_evaluated()</td><td>San Diego</td><td><i> </i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0602R4">P0602R4</a></td><td>LWG</td><td>variant and optional should propagate copy/move triviality</td><td>San Diego</td><td>Complete</td><td>8.0</td></tr>
   <tr><td><a href="https://wg21.link/P0608R3">P0608R3</a></td><td>LWG</td><td>A sane variant converting constructor</td><td>San Diego</td><td><i> </i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0655R1">P0655R1</a></td><td>LWG</td><td>visit&lt;R&gt;: Explicit Return Type for visit</td><td>San Diego</td><td><i> </i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0771R1">P0771R1</a></td><td>LWG</td><td>std::function move constructor should be noexcept</td><td>San Diego</td><td>Complete</td><td>6.0</td></tr>
   <tr><td><a href="https://wg21.link/P0896R4">P0896R4</a></td><td>LWG</td><td>The One Ranges Proposal</td><td>San Diego</td><td><i> </i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0899R1">P0899R1</a></td><td>LWG</td><td>P0899R1 - LWG 3016 is not a defect</td><td>San Diego</td><td><i>Nothing to do</i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0919R3">P0919R3</a></td><td>LWG</td><td>Heterogeneous lookup for unordered containers</td><td>San Diego</td><td><i> </i></td><td></td></tr>