Use allocator_traits to consistently allocate/deallocate/construct/destroy objects in std::any

[ldionne]

Summary

PR45099 notes (correctly) that we're inconsistent in memory allocation in std::any. We allocate memory with std::allocator<T>::allocate, construct with placement new, destroy by calling the destructor directly, and deallocate by calling delete. Most of those are customizable by the user - but in different ways.

The standard is silent on how these things are to be accomplished.
This patch makes it so we use allocator_traits<allocator<T>> for all of these operations (allocate, construct, destruct, deallocate). This is, at least, consistent.

[EricWF]

std::any, like other type erased types, cannot follow the allocator model and therefore does not use allocators.

The behavior of a user-provided specialization of allocator or allocator traits cannot differ meaningfully from the current code.
So I don't understand what the observable difference is here. Could you add a test?

libcxx/include/any

370

Can std::allocator have behavior that observably differs from this code? (which is just the default std::allocator inlined).

382

Is it the case that the allocator which allocated this memory is always equal to a default constructed allocator? If not, this is UB.

[EricWF]

All of that being said, this code is correct and LGTM**.

• Assuming the default constructed allocate is always equal to the allocator user to allocate the memory.

[mclow.lists]

So I don't understand what the observable difference is here. Could you add a test?

An observable difference:

The user has specialized std::allocator for his type (and it is 'large' for anys purposes).
When he puts one into an any, his allocator specialization is called to allocate the memory.
When the any is destroyed (or he puts something else into the any), then the global ::free is called.
This is potentially UB, since it passes free a pointer that didn't come from new.

There's a bunch of ways to do this. We should be consistent.
This is one way to be consistent. Another would be to use new/free everywhere.
I'm not really sure which is best. Since we were using the allocator before, I made it so we used it everywhere.

[mclow.lists]

libcxx/include/any

382

Remember that we're not talking about an arbitrary allocator, but std::allocator
(It can be a user specialization, but it's still std::allocator)

See http://eel.is/c++draft/allocator.globals. They're always equal.

[mclow.lists]

libcxx/include/any

370

Sure. A user-specialization of std::allocator can do most anything, as long as it actually constructs the object as well.

Register it in a global registry, for example.

[ldionne]

I like this change. It's simple but I think it makes sense. I'll add tests and finish it up.

[ldionne]

Add tests

[ldionne]

@tkoeppe Does this look like a suitable resolution to PR45099?

[tkoeppe]

In D81133#2274258, @ldionne wrote:

@tkoeppe Does this look like a suitable resolution to PR45099?

Without speaking for the dense implementationese, the direction looks good. Thanks!

[ldionne]

In D81133#2274279, @tkoeppe wrote:

In D81133#2274258, @ldionne wrote:

@tkoeppe Does this look like a suitable resolution to PR45099?

Without speaking for the dense implementationese, the direction looks good. Thanks!

Basically this will always use std::allocator<T> to allocate construct destroy deallocate inside std::any. Shipping!

libcxx/include/any

 
 #include <experimental/__config>
 #include <memory>
-#include <new>
 #include <typeinfo>
 #include <type_traits>
 #include <cstdlib>

     template <class ..._Args>
     _LIBCPP_INLINE_VISIBILITY
     static _Tp& __create(any & __dest, _Args&&... __args) {
-        _Tp* __ret = ::new (static_cast<void*>(&__dest.__s.__buf)) _Tp(_VSTD::forward<_Args>(__args)...);
+        typedef allocator<_Tp> _Alloc;
+        typedef allocator_traits<_Alloc> _ATraits;
+        _Alloc __a;
+        _Tp * __ret = static_cast<_Tp*>(static_cast<void*>(&__dest.__s.__buf));
+        _ATraits::construct(__a, __ret, _VSTD::forward<_Args>(__args)...);
         __dest.__h = &_SmallHandler::__handle;
         return *__ret;
     }

   private:
     _LIBCPP_INLINE_VISIBILITY
     static void __destroy(any & __this) {
-        _Tp & __value = *static_cast<_Tp *>(static_cast<void*>(&__this.__s.__buf));
-        __value.~_Tp();
+        typedef allocator<_Tp> _Alloc;
+        typedef allocator_traits<_Alloc> _ATraits;
+        _Alloc __a;
+        _Tp * __p = static_cast<_Tp *>(static_cast<void*>(&__this.__s.__buf));
+        _ATraits::destroy(__a, __p);
         __this.__h = nullptr;
     }
 

     _LIBCPP_INLINE_VISIBILITY
     static _Tp& __create(any & __dest, _Args&&... __args) {
         typedef allocator<_Tp> _Alloc;
+        typedef allocator_traits<_Alloc> _ATraits;
         typedef __allocator_destructor<_Alloc> _Dp;
         _Alloc __a;
-        unique_ptr<_Tp, _Dp> __hold(__a.allocate(1), _Dp(__a, 1));
-        _Tp* __ret = ::new ((void*)__hold.get()) _Tp(_VSTD::forward<_Args>(__args)...);
+        unique_ptr<_Tp, _Dp> __hold(_ATraits::allocate(__a, 1), _Dp(__a, 1));
+        _Tp * __ret = __hold.get();
+        _ATraits::construct(__a, __ret, _VSTD::forward<_Args>(__args)...);
         __dest.__s.__ptr = __hold.release();
         __dest.__h = &_LargeHandler::__handle;
         return *__ret;

 
     _LIBCPP_INLINE_VISIBILITY
     static void __destroy(any & __this){
-        delete static_cast<_Tp*>(__this.__s.__ptr);
+        typedef allocator<_Tp> _Alloc;
+        typedef allocator_traits<_Alloc> _ATraits;
+        _Alloc __a;
+        _Tp * __p = static_cast<_Tp *>(__this.__s.__ptr);
+        _ATraits::destroy(__a, __p);
+        _ATraits::deallocate(__a, __p, 1);
         __this.__h = nullptr;
     }