[libcxx] Use custom allocator's `construct` in C++03 when available.

[vsapsai]

Summary

Can use decltype in C++03 because include/__config defines a macro when
decltype is not available.

[• Quuxplusone]

I'm pretty sure that the C++03 standard doesn't permit the implementation to call any construct method here, even if it wanted to. And this adds a couple hundred LOC to get that (non-standard) behavior. Is there a specific use-case that you're trying to enable by adding this behavior to libc++?

[vsapsai]

In D48753#1147500, @Quuxplusone wrote:

I'm pretty sure that the C++03 standard doesn't permit the implementation to call any construct method here, even if it wanted to. And this adds a couple hundred LOC to get that (non-standard) behavior. Is there a specific use-case that you're trying to enable by adding this behavior to libc++?

Only the standard behaviour is required, so I need only

template <class _Tp, class _A0>
       static void construct(allocator_type& __a, _Tp* __p, const _A0& __a0);

to call allocator's construct. Other construct methods with different number of parameters were changed only for completeness. Also I'm thinking that using available matching allocator methods is something that developers would expect. But I don't think this opinion should influence decisions because seems there is no much use for custom allocator_type::construct.

If nobody thinks all variadic cases should be supported, I'll clean that up.

[vsapsai]

• Clean up functionality not required by the Standard.

[• Quuxplusone]

libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp

60 ↗

(On Diff #154018)

Per my comments on D48342, I think it would be fun to add a test allocator like

struct cpp03_fun_allocator : bare_allocator<T> {
    ...
    void construct(pointer p, const value_type& val) {
        construct(p, val, std::is_class<T>{});
    }
    void construct(pointer p, const value_type& val, std::true_type) {
        ::new(p) value_type(val);
    }
    void construct(pointer p, const value_type& val, std::false_type) {
        ::new(p) value_type(val);
    }

and just see whether it passes the test suite. If it does, might as well add that test. But if it doesn't, *I'm* not going to force anyone to fix it. Allocators in C++03 seems like masochism to me. :)

[vsapsai]

libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp

60 ↗

(On Diff #154018)

I've tweaked the test a little bit: added construct_called = true; in 2 places, used std::is_class<T>::value. That shouldn't change your intention. Tried and it works in C++17 but not in C++03.

Given that it didn't work earlier, not planning to fix it now.

[vsapsai]

• Use a better way to detect presence of construct with required signature. Clean up tests.

Don't know how other compilers will handle this but Clang accepts this
C++11-looking-but-accepted-in-C++03 code.

[• Quuxplusone]

libcxx/include/memory

1470 ↗

(On Diff #154326)

I think you should replace this ))) with )), void()) for absolute correctness (e.g. construct might plausibly return a pointer to the constructed object, and I think C++03 is okay with that).
Otherwise, awesome, this looks like what I'd expect. :)

[vsapsai]

libcxx/include/memory

1470 ↗

(On Diff #154326)

The standard says the result of construct is not used but I haven't found any mention it must be void.

That being said, the proposed change fails for the C++03 macro-based decltype shim:

error: too many arguments provided to function-like macro invocation
                                    _VSTD::declval<_Args>()), void()),
                                                              ^

One option to fix that is to use

is_same
<
    decltype(_as_before_),
    decltype(_the_same_copy_pasted_argument_)
>

But I don't think this use case is worth such hacks.

[• Quuxplusone]

libcxx/include/memory

1470 ↗

(On Diff #154326)

I think the workaround is easier. Try

template <class _Alloc, class _Pointer, class _A0>
struct __has_construct<_Alloc, _Pointer, _A0, typename enable_if<
  is_same<
    decltype((_VSTD::declval<_Alloc>().construct(
      _VSTD::declval<_Pointer>(),
      _VSTD::declval<_A0>()
    ), void() )),
    void
  >::value
>::type> : std::true_type {};

i.e. with an extra set of parens to group the argument to the decltype macro: decltype(()) instead of decltype(). I also drive-by edited Args to A0, matching how it's done elsewhere in pseudo-variadic-templates in this file.

[vsapsai]

• Allow allocator construct to return a value, not just have return type void.

[vsapsai]

libcxx/include/memory

1470 ↗

(On Diff #154326)

Thanks for the suggestion, Arthur, that worked.

Regarding _Args vs _A0, I am deliberately using _Args because it's not a manual pseudo-variadic-template expansion and it's not inside _LIBCPP_HAS_NO_VARIADICS block. But I don't feel strong about it, can change to _A0.

[EricWF]

Why are we doing this?

I can't find the language in the C++03 specification that requires us to call an allocators construct method if it's present.

[vsapsai]

In D48753#1157695, @EricWF wrote:

Why are we doing this?

I can't find the language in the C++03 specification that requires us to call an allocators construct method if it's present.

The main reason I've ended up doing this because I thought correct __has_construct is required for D48342. It's not the case but I think this change is useful on its own.

I agree that C++03 specification doesn't require to call construct method. Requirement for containers 23.1[lib.container.requirements]p9 mentions that allocator should be used for memory allocation:

Copy constructors for all container types defined in this clause copy an allocator argument from their respective first parameters. All other constructors for these container types take an Allocator& argument (20.1.6), an allocator whose value type is the same as the container’s value type. A copy of this argument is used for any memory allocation performed, by these constructors and by all member functions, during the lifetime of each container object. In all container types defined in this clause, the member get_allocator() returns a copy of the Allocator object used to construct the container.

And requirement for allocate 20.1.6[lib.allocator.requirements]table34 is not to construct objects:

Memory is allocated for n objects of type T but objects are not constructed.[...]

Also for specific containers you can have a requirement to construct a container using the specified allocator, without explaining what exactly does it mean. E.g. 23.2.1.1[lib.deque.cons]p3:

explicit deque(size_type n, const T& value = T(),
               const Allocator & = Allocator ());

Effects: Constructs a deque with n copies of value , using the specified allocator.

So these requirements tell that at some point you need to construct an object and you have to use an allocator but they don't explicitly spell out that you have to call construct.

My main motivation for this change is that in C++11 and later we do call construct if it is available and it is required per 23.2.1[container.requirements.general]p3:

For the components affected by this subclause that declare an allocator_type, objects stored in these components shall be constructed using the allocator_traits<allocator_type>::construct function and destroyed using the allocator_traits<allocator_type>::destroy function (20.7.8.2). These functions are called only for the container’s element type, not for internal types used by the container. [Note: This means, for example, that a node-based container might need to construct nodes containing aligned buffers and call construct to place the element into the buffer. — end note ]

I believe it is valuable and desirable to have the same behavior in different versions of C++ unless the semantic was explicitly changed. As far as I know, allocator_traits in C++11 wasn't supposed to be a breaking change.

[• Quuxplusone]

In D48753#1157695, @EricWF wrote:

Why are we doing this?

I can't find the language in the C++03 specification that requires us to call an allocators construct method if it's present.

I think it's being proposed under "quality of implementation."

It has only just now occurred to me: a quality implementation should probably never mismatch calls to construct and destroy. Does libc++ currently call destroy in C++03? Does it call destroy in C++03 after this patch? If this patch is making libc++ call construct but not destroy, that's a bug. If before this patch libc++ would call destroy but not construct, this patch is a bugfix! (And please add a test case for matched pairs of construct and destroy.)

libcxx/include/memory

1470 ↗

(On Diff #154326)

Please change it (as far as I'm concerned, anyway). "_Args" plural implies a pack, and there's no pack here.

[vsapsai]

In D48753#1157907, @Quuxplusone wrote:

In D48753#1157695, @EricWF wrote:

Why are we doing this?

I can't find the language in the C++03 specification that requires us to call an allocators construct method if it's present.

I think it's being proposed under "quality of implementation."

It has only just now occurred to me: a quality implementation should probably never mismatch calls to construct and destroy. Does libc++ currently call destroy in C++03? Does it call destroy in C++03 after this patch? If this patch is making libc++ call construct but not destroy, that's a bug. If before this patch libc++ would call destroy but not construct, this patch is a bugfix! (And please add a test case for matched pairs of construct and destroy.)

In C++03 destroy isn't called neither before my patch nor after. Will fix that. Though it's harder to find specification for destroy in C++03.

[vsapsai]

• In C++03 call allocator's destroy when available.
• Rename _Args as it's not a variadic template pack.

[vsapsai]

Ping.

[ldionne]

1. Just to make sure I understand; this patch has nothing to do with https://reviews.llvm.org/D48342, they are entirely orthogonal. Is this correct?
2. Also, before this patch, the allocator's construct and destroy were NEVER called in C++03, but were called when available in C++11. After this patch, they are called when available in C++03 and in C++11. Is this correct?

If (2) is true, then I believe this patch is a worthwhile improvement in terms of quality-of-implementation, even though it's not mandated by the spec. People do have custom allocators, and this behavior change between C++03 and C++11 is quite subtle and surprising.

libcxx/include/memory

1466 ↗

(On Diff #154912)

You can just say

template <class _Alloc, class _Pointer, class _Tp>
struct __has_construct<_Alloc, _Pointer, _Tp, typename __void_t<
    decltype(_VSTD::declval<_Alloc>().construct(_VSTD::declval<_Pointer>(), _VSTD::declval<_Tp>()))
>::type> : std::true_type { };

1482 ↗

(On Diff #154912)

Here,

template <class _Alloc, class _Pointer>
struct __has_destroy<_Alloc, _Pointer, typename __void_t<
    decltype(_VSTD::declval<_Alloc>().destroy(_VSTD::declval<_Pointer>()))
>::type> : std::true_type { };

1726 ↗

(On Diff #154912)

Here,

template <class _Tp, class _A0, class = typename enable_if<__has_construct<allocator_type, _Tp*, _A0 const&>::value>::type>
static void __construct(...);

and then below

template <class _Tp, class _A0, class = typename enable_if<!__has_construct<allocator_type, _Tp*, _A0 const&>::value>::type>
static void __construct(...);

This way you don't have to call __has_construct at the point of call and __construct is slightly more reusable.

libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp

122 ↗

(On Diff #154912)

These tests are specific to libc++ because C++03 does not mandate this behavior. Please either move them into test/libcxx or guard them with _LIBCPP_VERSION. I think the former is what we normally do in these cases and should be preferred.

libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp

135 ↗

(On Diff #154912)

Same as above, this is libc++ specific.

[vsapsai]

• Update __has_construct, __has_destroy according to review comments.
• Tighten tests: custom allocators aren't mandated by C++03 but libc++ has the support.

[vsapsai]

Regarding the tests. I've moved most of new tests for custom allocators to test/libcxx/*. And in destroy.pass.cpp I'm just checking _LIBCPP_VERSION to avoid copying the test to a different file.

libcxx/include/memory

1466 ↗

(On Diff #154912)

Done. One of the reasons to use is_same was to mimic __has_construct for C++11 and later. But I don't think there is really value in keeping that.

1726 ↗

(On Diff #154912)

I've tried that (also updated how static void construct calls __construct) and it works with C++2a but fails with C++03. The error is

llvm-project/libcxx/include/memory:1725:21: error: class member cannot be redeclared
        static void __construct(allocator_type&, _Tp* __p, const _A0& __a0)
                    ^
llvm-project/libcxx/include/memory:1720:21: note: previous definition is here
        static void __construct(allocator_type& __a, _Tp* __p, const _A0& __a0)
                    ^

I haven't investigated further yet.

[• Quuxplusone]

libcxx/include/memory

1726 ↗

(On Diff #154912)

FWIW, I don't have a strong opinion here. I think Volodymyr's way instantiates slightly fewer intermediate types. It's also consistent with the definition of __destroy right below it. I don't know who we expect to be "reusing" allocator_traits<A>::__construct.

OTOH, in this case I don't see any concrete benefit to making the dispatch explicit at the callsite. (Whereas in D49317 (Ctrl+F "Got it") I needed the dispatch to be explicit at the callsite.) So refactoring __construct would be reasonable, although perhaps slower, and open a can of worms as to whether __destroy should also be refactored.

[vsapsai]

In D48753#1323248, @ldionne wrote:

2. Also, before this patch, the allocator's construct and destroy were NEVER called in C++03, but were called when available in C++11. After this patch, they are called when available in C++03 and in C++11. Is this correct?

If (2) is true, then I believe this patch is a worthwhile improvement in terms of quality-of-implementation, even though it's not mandated by the spec. People do have custom allocators, and this behavior change between C++03 and C++11 is quite subtle and surprising.

The described situation is correct. And I agree it can be surprising and tricky.

1. Just to make sure I understand; this patch has nothing to do with https://reviews.llvm.org/D48342, they are entirely orthogonal. Is this correct?

They are not entirely orthogonal in sense that https://reviews.llvm.org/D48342 doesn't account for difference in allocator behaviour between C++03 and C++11. So it is better to have uniform construct behaviour before using it in implementing other fixes.

[vsapsai]

Ping.

libcxx/trunk/include/memory

: integral_constant<bool,
         is_same<
             decltype(_VSTD::__has_select_on_container_copy_construction_test(declval<_Alloc&>())),
             true_type>::value>
 {
 };
 
 #else  // _LIBCPP_CXX03_LANG
 
-#ifndef _LIBCPP_HAS_NO_VARIADICS
-
-template <class _Alloc, class _Pointer, class ..._Args>
-struct __has_construct
-    : false_type
-{
-};
+template <class _Alloc, class _Pointer, class _Tp, class = void>
+struct __has_construct : std::false_type {};
 
-#else  // _LIBCPP_HAS_NO_VARIADICS
+template <class _Alloc, class _Pointer, class _Tp>
+struct __has_construct<_Alloc, _Pointer, _Tp, typename __void_t<
+    decltype(_VSTD::declval<_Alloc>().construct(_VSTD::declval<_Pointer>(), _VSTD::declval<_Tp>()))
+>::type> : std::true_type {};
 
-template <class _Alloc, class _Pointer, class _Args>
-struct __has_construct
-    : false_type
-{
-};
-
-#endif  // _LIBCPP_HAS_NO_VARIADICS
+template <class _Alloc, class _Pointer, class = void>
+struct __has_destroy : false_type {};
 
 template <class _Alloc, class _Pointer>
-struct __has_destroy
-    : false_type
-{
-};
+struct __has_destroy<_Alloc, _Pointer, typename __void_t<
+    decltype(_VSTD::declval<_Alloc>().destroy(_VSTD::declval<_Pointer>()))
+>::type> : std::true_type {};
 
 template <class _Alloc>
 struct __has_max_size
     : true_type
 {
 };
 
 template <class _Alloc>
#else  // _LIBCPP_HAS_NO_VARIADICS
     template <class _Tp>
         _LIBCPP_INLINE_VISIBILITY
         static void construct(allocator_type&, _Tp* __p)
             {
                 ::new ((void*)__p) _Tp();
             }
     template <class _Tp, class _A0>
         _LIBCPP_INLINE_VISIBILITY
-        static void construct(allocator_type&, _Tp* __p, const _A0& __a0)
+        static void construct(allocator_type& __a, _Tp* __p, const _A0& __a0)
             {
-                ::new ((void*)__p) _Tp(__a0);
+                __construct(__has_construct<allocator_type, _Tp*, const _A0&>(),
+                            __a, __p, __a0);
             }
     template <class _Tp, class _A0, class _A1>
         _LIBCPP_INLINE_VISIBILITY
         static void construct(allocator_type&, _Tp* __p, const _A0& __a0,
                               const _A1& __a1)
             {
                 ::new ((void*)__p) _Tp(__a0, __a1);
             }
template <class _Tp, class... _Args>
         static void __construct(true_type, allocator_type& __a, _Tp* __p, _Args&&... __args)
             {__a.construct(__p, _VSTD::forward<_Args>(__args)...);}
     template <class _Tp, class... _Args>
         _LIBCPP_INLINE_VISIBILITY
         static void __construct(false_type, allocator_type&, _Tp* __p, _Args&&... __args)
             {
                 ::new ((void*)__p) _Tp(_VSTD::forward<_Args>(__args)...);
             }
+#else  // _LIBCPP_HAS_NO_VARIADICS
+    template <class _Tp, class _A0>
+        _LIBCPP_INLINE_VISIBILITY
+        static void __construct(true_type, allocator_type& __a, _Tp* __p,
+                                const _A0& __a0)
+            {__a.construct(__p, __a0);}
+    template <class _Tp, class _A0>
+        _LIBCPP_INLINE_VISIBILITY
+        static void __construct(false_type, allocator_type&, _Tp* __p,
+                                const _A0& __a0)
+            {
+                ::new ((void*)__p) _Tp(__a0);
+            }
 #endif  // _LIBCPP_HAS_NO_VARIADICS
 
     template <class _Tp>
         _LIBCPP_INLINE_VISIBILITY
         static void __destroy(true_type, allocator_type& __a, _Tp* __p)
             {__a.destroy(__p);}
     template <class _Tp>
         _LIBCPP_INLINE_VISIBILITY

libcxx/trunk/test/libcxx/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+// <vector>
+
+// template <class InputIter> vector(InputIter first, InputIter last);
+
+#include <vector>
+#include <cassert>
+
+#include "min_allocator.h"
+
+void test_ctor_under_alloc() {
+  int arr1[] = {42};
+  int arr2[] = {1, 101, 42};
+  {
+    typedef std::vector<int, cpp03_allocator<int> > C;
+    typedef C::allocator_type Alloc;
+    {
+      Alloc::construct_called = false;
+      C v(arr1, arr1 + 1);
+      assert(Alloc::construct_called);
+    }
+    {
+      Alloc::construct_called = false;
+      C v(arr2, arr2 + 3);
+      assert(Alloc::construct_called);
+    }
+  }
+  {
+    typedef std::vector<int, cpp03_overload_allocator<int> > C;
+    typedef C::allocator_type Alloc;
+    {
+      Alloc::construct_called = false;
+      C v(arr1, arr1 + 1);
+      assert(Alloc::construct_called);
+    }
+    {
+      Alloc::construct_called = false;
+      C v(arr2, arr2 + 3);
+      assert(Alloc::construct_called);
+    }
+  }
+}
+
+int main() {
+  test_ctor_under_alloc();
+}

libcxx/trunk/test/libcxx/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+// <vector>
+
+// template <class InputIter> vector(InputIter first, InputIter last,
+//                                   const allocator_type& a);
+
+#include <vector>
+#include <cassert>
+
+#include "min_allocator.h"
+
+void test_ctor_under_alloc() {
+  int arr1[] = {42};
+  int arr2[] = {1, 101, 42};
+  {
+    typedef std::vector<int, cpp03_allocator<int> > C;
+    typedef C::allocator_type Alloc;
+    Alloc a;
+    {
+      Alloc::construct_called = false;
+      C v(arr1, arr1 + 1, a);
+      assert(Alloc::construct_called);
+    }
+    {
+      Alloc::construct_called = false;
+      C v(arr2, arr2 + 3, a);
+      assert(Alloc::construct_called);
+    }
+  }
+  {
+    typedef std::vector<int, cpp03_overload_allocator<int> > C;
+    typedef C::allocator_type Alloc;
+    Alloc a;
+    {
+      Alloc::construct_called = false;
+      C v(arr1, arr1 + 1, a);
+      assert(Alloc::construct_called);
+    }
+    {
+      Alloc::construct_called = false;
+      C v(arr2, arr2 + 3, a);
+      assert(Alloc::construct_called);
+    }
+  }
+}
+
+int main() {
+  test_ctor_under_alloc();
+}

libcxx/trunk/test/std/utilities/memory/allocator.traits/allocator.traits.members/destroy.pass.cpp

int main()
     }
     {
       typedef IncompleteHolder* VT;
       typedef A<VT> Alloc;
       Alloc a;
       std::aligned_storage<sizeof(VT)>::type store;
       std::allocator_traits<Alloc>::destroy(a, (VT*)&store);
     }
-#if TEST_STD_VER >= 11
+#if defined(_LIBCPP_VERSION) || TEST_STD_VER >= 11
     {
         A0::count = 0;
         b_destroy = 0;
         B<int> b;
         std::aligned_storage<sizeof(A0)>::type a0;
         std::allocator_traits<B<int> >::construct(b, (A0*)&a0);
         assert(A0::count == 0);
         assert(b_destroy == 0);
         std::allocator_traits<B<int> >::destroy(b, (A0*)&a0);
         assert(A0::count == 1);
         assert(b_destroy == 1);
     }
 #endif
 }

libcxx/trunk/test/support/min_allocator.h

 //===----------------------------------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is dual licensed under the MIT and the University of Illinois Open
 // Source Licenses. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef MIN_ALLOCATOR_H
 #define MIN_ALLOCATOR_H
 
 #include <cstddef>
 #include <cstdlib>
 #include <cstddef>
 #include <cassert>
+#include <climits>
 
 #include "test_macros.h"
 
 template <class T>
 class bare_allocator
 {
 public:
     typedef T value_type;
void deallocate(T* p, std::size_t)
         ++dealloc_count;
         std::free(static_cast<void*>(p));
     }
 
     friend bool operator==(malloc_allocator, malloc_allocator) {return true;}
     friend bool operator!=(malloc_allocator x, malloc_allocator y) {return !(x == y);}
 };
 
+template <class T>
+struct cpp03_allocator : bare_allocator<T>
+{
+    typedef T value_type;
+    typedef value_type* pointer;
+
+    static bool construct_called;
+
+    // Returned value is not used but it's not prohibited.
+    pointer construct(pointer p, const value_type& val)
+    {
+        ::new(p) value_type(val);
+        construct_called = true;
+        return p;
+    }
+
+    std::size_t max_size() const
+    {
+        return UINT_MAX / sizeof(T);
+    }
+};
+template <class T> bool cpp03_allocator<T>::construct_called = false;
+
+template <class T>
+struct cpp03_overload_allocator : bare_allocator<T>
+{
+    typedef T value_type;
+    typedef value_type* pointer;
+
+    static bool construct_called;
+
+    void construct(pointer p, const value_type& val)
+    {
+        construct(p, val, std::is_class<T>());
+    }
+    void construct(pointer p, const value_type& val, std::true_type)
+    {
+        ::new(p) value_type(val);
+        construct_called = true;
+    }
+    void construct(pointer p, const value_type& val, std::false_type)
+    {
+        ::new(p) value_type(val);
+        construct_called = true;
+    }
+
+    std::size_t max_size() const
+    {
+        return UINT_MAX / sizeof(T);
+    }
+};
+template <class T> bool cpp03_overload_allocator<T>::construct_called = false;
+
 
 #if TEST_STD_VER >= 11
 
 #include <memory>
 
 template <class T, class = std::integral_constant<size_t, 0> > class min_pointer;
 template <class T, class ID> class min_pointer<const T, ID>;
 template <class ID> class min_pointer<void, ID>;