[libc++] NFCI: Implement make_shared as allocate_shared with std::allocator

[ldionne]

Summary

This simplifies the implementation, and it appears to be equivalent since
make_shared was allocating memory with std::allocator anyway.

[ldionne]

@glenjofe Taking a chance to tag you here since you've been involved in most changes to std::shared_ptr (and Peter Dimov does not appear to be on Phabricator). Was the intent that make_shared could be implemented via allocate_shared with the default allocator, or am I missing something subtle?

[mclow.lists]

if you allocate memory with an allocator, you need to deallocate it with the same allocator. Not with operator delete

[mclow.lists]

In D93071#2447271, @mclow.lists wrote:

if you allocate memory with an allocator, you need to deallocate it with the same allocator. Not with operator delete

There's a good chance that this is a pre-existing problem.

[ldionne]

In D93071#2447273, @mclow.lists wrote:

In D93071#2447271, @mclow.lists wrote:

if you allocate memory with an allocator, you need to deallocate it with the same allocator. Not with operator delete

There's a good chance that this is a pre-existing problem.

Indeed, however std::allocator is specified to use operator new and operator delete. Also, we already did allocate using std::allocator<>::allocate and our control block already deallocates using the provided allocator's deallocate function, so the allocate and deallocate calls are matched (both before and after the patch).

[mclow.lists]

In D93071#2447283, @ldionne wrote:

Indeed, however std::allocator is specified to use operator new and operator delete.

I don't think that applies to user-provided specializations of std::allocator

[zoecarver]

Thanks for the cleanup. This LGTM!

[ldionne]

In D93071#2447413, @mclow.lists wrote:

In D93071#2447283, @ldionne wrote:

Indeed, however std::allocator is specified to use operator new and operator delete.

I don't think that applies to user-provided specializations of std::allocator

Good point, I hadn't thought about specializations. But even then, I think it's fine. User-provided specializations are allowed as long as they depend on a user-defined type, and they satisfy the requirements of the base template. Here, std::allocator::allocate clearly says:

Allocates n * sizeof(T) bytes of uninitialized storage by calling ::operator new(std::size_t)

IDK whether that's considered "a requirement", but it's close enough. In any case, if it is an issue, then it's a pre-existing condition because we were using std::allocator already, so I think this is really a NFC. I just hope we're not violating the intent of the standard, but as I think of it, I believe we couldn't change that behavior without an ABI break (because it would require changing the control block used by make_shared so that it doesn't use an allocator anymore.

[glenjofe]

In D93071#2447152, @ldionne wrote:

@glenjofe Taking a chance to tag you here since you've been involved in most changes to std::shared_ptr (and Peter Dimov does not appear to be on Phabricator). Was the intent that make_shared could be implemented via allocate_shared with the default allocator, or am I missing something subtle?

My implementation Boost's does: https://github.com/boostorg/smart_ptr/blob/develop/include/boost/smart_ptr/make_shared_array.hpp

i.e. Those boost::make_shared overloads just call boost::allocate_shared with boost::default_allocator

It would be nice to do specify std::make_shared overloads in the standard as just returning std::allocate_shared with std::allocator but we would need wording for that.

As pointed out users can specialize std::allocator and some even specialize std::allocator_traits.

[ldionne]

In D93071#2456926, @glenjofe wrote:

In D93071#2447152, @ldionne wrote:

@glenjofe Taking a chance to tag you here since you've been involved in most changes to std::shared_ptr (and Peter Dimov does not appear to be on Phabricator). Was the intent that make_shared could be implemented via allocate_shared with the default allocator, or am I missing something subtle?

My implementation Boost's does: https://github.com/boostorg/smart_ptr/blob/develop/include/boost/smart_ptr/make_shared_array.hpp

i.e. Those boost::make_shared overloads just call boost::allocate_shared with boost::default_allocator

It would be nice to do specify std::make_shared overloads in the standard as just returning std::allocate_shared with std::allocator but we would need wording for that.

As pointed out users can specialize std::allocator and some even specialize std::allocator_traits.

Thanks for the clarification of intent! I believe it's a reasonable implementation -- I can hardly imagine someone relying on std::allocator *not* being used by std::make_shared. Or at least, that would imply a very specific way of reading of the standard.

libcxx/include/memory

<
     __compatible_with<_Yp, typename shared_ptr<_Tp>::element_type>::value,
     void
 >::type
 shared_ptr<_Tp>::reset(_Yp* __p, _Dp __d, _Alloc __a)
 {
     shared_ptr(__p, __d, __a).swap(*this);
 }
 
-template<class _Tp, class ..._Args>
-inline _LIBCPP_INLINE_VISIBILITY
-typename enable_if
-<
-    !is_array<_Tp>::value,
-    shared_ptr<_Tp>
->::type
-make_shared(_Args&& ...__args)
-{
-    static_assert(is_constructible<_Tp, _Args...>::value, "Can't construct object in make_shared");
-    typedef __shared_ptr_emplace<_Tp, allocator<_Tp> > _CntrlBlk;
-    typedef allocator<_CntrlBlk> _A2;
-    typedef __allocator_destructor<_A2> _D2;
-
-    _A2 __a2;
-    unique_ptr<_CntrlBlk, _D2> __hold2(__a2.allocate(1), _D2(__a2, 1));
-    ::new(__hold2.get()) _CntrlBlk(__a2, _VSTD::forward<_Args>(__args)...);
-
-    _Tp *__ptr = __hold2->__get_elem();
-    return shared_ptr<_Tp>::__create_with_control_block(__ptr, __hold2.release());
-}
-
 template<class _Tp, class _Alloc, class ..._Args>
 inline _LIBCPP_INLINE_VISIBILITY
 typename enable_if
 <
     !is_array<_Tp>::value,
     shared_ptr<_Tp>
 >::type
 allocate_shared(const _Alloc& __a, _Args&& ...__args)
 {
-    static_assert( is_constructible<_Tp, _Args...>::value, "Can't construct object in allocate_shared");
+    static_assert(is_constructible<_Tp, _Args...>::value,
+        "allocate_shared/make_shared: the type is not constructible from the provided arguments");
 
     typedef __shared_ptr_emplace<_Tp, _Alloc> _CntrlBlk;
     typedef typename __allocator_traits_rebind<_Alloc, _CntrlBlk>::type _A2;
     typedef __allocator_destructor<_A2> _D2;
 
     _A2 __a2(__a);
     unique_ptr<_CntrlBlk, _D2> __hold2(__a2.allocate(1), _D2(__a2, 1));
     ::new(static_cast<void*>(_VSTD::addressof(*__hold2.get())))
         _CntrlBlk(__a, _VSTD::forward<_Args>(__args)...);
 
     typename shared_ptr<_Tp>::element_type *__p = __hold2->__get_elem();
     return shared_ptr<_Tp>::__create_with_control_block(__p, _VSTD::addressof(*__hold2.release()));
 }
 
+template<class _Tp, class ..._Args, class = _EnableIf<!is_array<_Tp>::value> >
+_LIBCPP_HIDE_FROM_ABI
+shared_ptr<_Tp> make_shared(_Args&& ...__args)
+{
+    return _VSTD::allocate_shared<_Tp>(allocator<_Tp>(), _VSTD::forward<_Args>(__args)...);
+}
+
 template<class _Tp, class _Up>
 inline _LIBCPP_INLINE_VISIBILITY
 bool
 operator==(const shared_ptr<_Tp>& __x, const shared_ptr<_Up>& __y) _NOEXCEPT
 {
     return __x.get() == __y.get();
 }
 

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared.protected.verify.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
 //
 //===----------------------------------------------------------------------===//
 
 // GCC 5 does not evaluate static assertions dependent on a template parameter.
 // UNSUPPORTED: gcc-5
 
 // <memory>
 
 // shared_ptr
 
-// template<class T, class... Args> shared_ptr<T> make_shared(Args&&... args);
+// template<class T, class A, class... Args>
+//    shared_ptr<T> allocate_shared(const A& a, Args&&... args);
 
 #include <memory>
-#include <cassert>
 
 #include "test_macros.h"
 
 struct S {
 protected:
    S () {};  // ctor is protected
 };
 
-int main(int, char**)
-{
-  std::shared_ptr<S> p = std::make_shared<
-      S>(); // expected-error@memory:* {{static_assert failed due to requirement 'is_constructible<S>::value' "Can't construct object in make_shared"}}
+int main(int, char**) {
+    typedef std::allocator<S> A;
+    A a;
+    std::shared_ptr<S> p = std::allocate_shared<S, A>(a); // expected-error@memory:* {{static_assert failed due to requirement 'is_constructible<S>::value}}
 
     return 0;
 }

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/make_shared.protected.verify.cpp

 
 // <memory>
 
 // shared_ptr
 
 // template<class T, class... Args> shared_ptr<T> make_shared(Args&&... args);
 
 #include <memory>
-#include <cassert>
 
 #include "test_macros.h"
 
 struct S {
 protected:
    S () {};  // ctor is protected
 };
 
-int main(int, char**)
-{
-  std::shared_ptr<S> p = std::make_shared<
-      S>(); // expected-error@memory:* {{static_assert failed due to requirement 'is_constructible<S>::value' "Can't construct object in make_shared"}}
+int main(int, char**) {
+  std::shared_ptr<S> p = std::make_shared<S>(); // expected-error@memory:* {{static_assert failed due to requirement 'is_constructible<S>::value}}
 
   return 0;
 }