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

template<class ValueType>
     ValueType* any_cast(any* operand) noexcept;
 
 } // namespace std
 
 */
 
 #include <experimental/__config>
 #include <memory>
-#include <new>
 #include <typeinfo>
 #include <type_traits>
 #include <cstdlib>
 #include <version>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
static void* __handle(_Action __act, any const * __this, any * __other,
         case _Action::_TypeInfo:
           return __type_info();
         }
     }
 
     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 void __copy(any const & __this, any & __dest) {
         _SmallHandler::__create(__dest, *static_cast<_Tp const *>(
             static_cast<void const *>(&__this.__s.__buf)));
     }
static void* __handle(_Action __act, any const * __this,
           return __type_info();
         }
     }
 
     template <class ..._Args>
     _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;
     }
 
   private:
 
     _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;
     }
 
     _LIBCPP_INLINE_VISIBILITY
     static void __copy(any const & __this, any & __dest) {
         _LargeHandler::__create(__dest, *static_cast<_Tp const *>(__this.__s.__ptr));
     }
 

libcxx/test/libcxx/utilities/any/allocator.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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14
+
+// <any>
+
+// Check that we're consistently using std::allocator_traits to
+// allocate/deallocate/construct/destroy objects in std::any.
+// See https://llvm.org/PR45099 for details.
+
+#include <any>
+#include <cassert>
+#include <cstddef>
+#include <memory>
+#include <type_traits>
+#include <utility>
+
+#include "test_macros.h"
+
+
+// Make sure we don't fit in std::any's SBO
+struct Large { char big[sizeof(std::any) + 1]; };
+
+// Make sure we fit in std::any's SBO
+struct Small { };
+
+bool Large_was_allocated = false;
+bool Large_was_constructed = false;
+bool Large_was_destroyed = false;
+bool Large_was_deallocated = false;
+
+bool Small_was_allocated = false;
+bool Small_was_constructed = false;
+bool Small_was_destroyed = false;
+bool Small_was_deallocated = false;
+
+namespace std {
+  template <>
+  struct allocator<Large> {
+    using value_type = Large;
+    using size_type = std::size_t;
+    using difference_type = std::ptrdiff_t;
+    using propagate_on_container_move_assignment = std::true_type;
+    using is_always_equal = std::true_type;
+
+    Large* allocate(std::size_t n) {
+      Large_was_allocated = true;
+      return static_cast<Large*>(::operator new(n));
+    }
+
+    template <typename ...Args>
+    void construct(Large* p, Args&& ...args) {
+      new (p) Large(std::forward<Args>(args)...);
+      Large_was_constructed = true;
+    }
+
+    void destroy(Large* p) {
+      p->~Large();
+      Large_was_destroyed = true;
+    }
+
+    void deallocate(Large* p, std::size_t) {
+      Large_was_deallocated = true;
+      return ::operator delete(p);
+    }
+  };
+
+  template <>
+  struct allocator<Small> {
+    using value_type = Small;
+    using size_type = std::size_t;
+    using difference_type = std::ptrdiff_t;
+    using propagate_on_container_move_assignment = std::true_type;
+    using is_always_equal = std::true_type;
+
+    Small* allocate(std::size_t n) {
+      Small_was_allocated = true;
+      return static_cast<Small*>(::operator new(n));
+    }
+
+    template <typename ...Args>
+    void construct(Small* p, Args&& ...args) {
+      new (p) Small(std::forward<Args>(args)...);
+      Small_was_constructed = true;
+    }
+
+    void destroy(Small* p) {
+      p->~Small();
+      Small_was_destroyed = true;
+    }
+
+    void deallocate(Small* p, std::size_t) {
+      Small_was_deallocated = true;
+      return ::operator delete(p);
+    }
+  };
+} // end namespace std
+
+
+int main(int, char**) {
+  // Test large types
+  {
+    {
+      std::any a = Large();
+      (void)a;
+
+      assert(Large_was_allocated);
+      assert(Large_was_constructed);
+    }
+
+    assert(Large_was_destroyed);
+    assert(Large_was_deallocated);
+  }
+
+  // Test small types
+  {
+    {
+      std::any a = Small();
+      (void)a;
+
+      assert(!Small_was_allocated);
+      assert(Small_was_constructed);
+    }
+
+    assert(Small_was_destroyed);
+    assert(!Small_was_deallocated);
+  }
+
+  return 0;
+}