Update shared_ptr's constructor

[zoecarver]

Summary

This patch represents the ethos of D62233. It is much smaller and will hopefully be easier to review while still achieving the same result :)

[zoecarver]

Another friendly ping :) I would like to get this going because other patches rely on it. I have patches to resolve bugs, issues, and features. @mclow.lists @EricWF @ldionne

[zoecarver]

Weekly ping ;)

[zoecarver]

Ping. Thanks for reviewing the other patches, this is the last in the series.

[ldionne]

libcxx/include/memory

3982

Before, we would create the control block with new, and with this patch we're using the allocator. Is this correct? Why?

Also, we're now unconditionally using a try-catch block -- I'd be curious to see what the impact on code generation is.

4012

Note to reviewers: It's okay to move this outside of the try-catch block, since __enable_weak_this is noexcept anyway.

[zoecarver]

libcxx/include/memory

3982

Yes, it's correct. The allocator will call __builtin_operator_new (or operator new), which makes it functionally equivalent. When the control block is deallocated, it calls __a.deallocate. I think it's better that the control block is also allocated with the allocator (even if they're functionally equivalent) both for cognitive load and in case we introduced an optimization (for example) that broke this type of deallocation down the road.

I'll take a look at the codegen.

[zoecarver]

libcxx/include/memory

3982

Yes, the try-catch-block really hurts the codegen. With and without (18 vs 2 lines of assembly).

[zoecarver]

libcxx/include/memory

3930

I'm going to move the try-catch-blocks out of these two members and into the initializers they're used in. This should help with codegen.

Also, I forgot to wrap these in the #ifs.

[zoecarver]

• Move try-catch outside of __allocate_shared

[zoecarver]

@ldionne ping.

[zoecarver]

Looking at it again, I'm not sure this patch is really needed. None of my other subsequent patches depend on it. If you want, I'm happy to abandon it. I do think it's not a bad cleanup, though. Anyway, I'd like to close it one way or another (either by committing it or by abandoning it).

cc @ldionne

[zoecarver]

I'm going to rebase D62259 off master instead of this patch. I'll close this patch for now if you think it would be a good change, I can re-open it later.

libcxx/include/memory

template <class _Yp, class _OrigPtr>
                     const_cast<_RawYp*>(static_cast<const _Yp*>(__ptr)));
             }
         }
 
     _LIBCPP_INLINE_VISIBILITY void __enable_weak_this(...) _NOEXCEPT {}
 
     template <class _Up> friend class _LIBCPP_TEMPLATE_VIS shared_ptr;
     template <class _Up> friend class _LIBCPP_TEMPLATE_VIS weak_ptr;
+
+    template<class _Yp, class _Deleter = default_delete<_Yp>,
+                        class _Alloc = typename __shared_ptr_default_allocator<_Yp>::type>
+    static __shared_weak_count*
+    __allocate_shared(_Yp* __p, _Deleter __d = _Deleter(), _Alloc __a = _Alloc())
+    {
+        typedef __shared_ptr_pointer<_Yp*, _Deleter, _Alloc> _CT;
+        typedef typename __allocator_traits_rebind<_Alloc, _CT>::type _A2;
+        typedef __allocator_destructor<_A2> _D2;
+
+        _A2 __a2(__a);
+        unique_ptr<_CT, _D2> __hold2(__a2.allocate(1), _D2(__a2, 1));
+
+        ::new(static_cast<void*>(_VSTD::addressof(*__hold2.get())))
+            _CT(__p, __d, __a);
+
+        return _VSTD::addressof(*__hold2.release());
+    }
+
+    template<class _Deleter = default_delete<_Tp>,
+             class _Alloc = typename __shared_ptr_default_allocator<_Tp>::type>
+    static __shared_weak_count*
+    __allocate_shared(nullptr_t __p, _Deleter __d = _Deleter(), _Alloc __a = _Alloc())
+    {
+        typedef __shared_ptr_pointer<_Tp*, _Deleter, _Alloc> _CT;
+        typedef typename __allocator_traits_rebind<_Alloc, _CT>::type _A2;
+        typedef __allocator_destructor<_A2> _D2;
+
+        _A2 __a2(__a);
+        unique_ptr<_CT, _D2> __hold2(__a2.allocate(1), _D2(__a2, 1));
+
+        ::new(static_cast<void*>(_VSTD::addressof(*__hold2.get())))
+            _CT(__p, __d, __a);
+
+        return _VSTD::addressof(*__hold2.release());
+    }
 };
 
 
 template<class _Tp>
 inline
 _LIBCPP_CONSTEXPR
 shared_ptr<_Tp>::shared_ptr() _NOEXCEPT
     : __ptr_(0),
-      __cntrl_(0)
-{
-}
+      __cntrl_(0) {}
 
 template<class _Tp>
 inline
 _LIBCPP_CONSTEXPR
 shared_ptr<_Tp>::shared_ptr(nullptr_t) _NOEXCEPT
     : __ptr_(0),
-      __cntrl_(0)
-{
-}
+      __cntrl_(0) {}
 
 template<class _Tp>
 template<class _Yp>
 shared_ptr<_Tp>::shared_ptr(_Yp* __p,
                             typename enable_if<is_convertible<_Yp*, element_type*>::value, __nat>::type)
     : __ptr_(__p)
 {
-    unique_ptr<_Yp> __hold(__p);
-    typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
-    typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, _AllocT > _CntrlBlk;
-    __cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), _AllocT());
-    __hold.release();
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    try {
+#endif  // _LIBCPP_NO_EXCEPTIONS
+      __cntrl_ = __allocate_shared(__p);
       __enable_weak_this(__p, __p);
+#ifndef _LIBCPP_NO_EXCEPTIONS
+    } catch (...) {
+        default_delete<_Yp>()(__p);
+        throw;
+    }
+#endif  // _LIBCPP_NO_EXCEPTIONS
 }
 
 template<class _Tp>
 template<class _Yp, class _Dp>
 shared_ptr<_Tp>::shared_ptr(_Yp* __p, _Dp __d,
                             typename enable_if<is_convertible<_Yp*, element_type*>::value, __nat>::type)
     : __ptr_(__p)
 {
 #ifndef _LIBCPP_NO_EXCEPTIONS
-    try
-    {
+    try {
 #endif  // _LIBCPP_NO_EXCEPTIONS
-        typedef typename __shared_ptr_default_allocator<_Yp>::type _AllocT;
-        typedef __shared_ptr_pointer<_Yp*, _Dp, _AllocT > _CntrlBlk;
-        __cntrl_ = new _CntrlBlk(__p, __d, _AllocT());
-        __enable_weak_this(__p, __p);
+      __cntrl_ = __allocate_shared(__p, __d);
 #ifndef _LIBCPP_NO_EXCEPTIONS
-    }
-    catch (...)
-    {
+    } catch (...) {
         __d(__p);
         throw;
     }
 #endif  // _LIBCPP_NO_EXCEPTIONS
 }
 
 template<class _Tp>
 template<class _Dp>
 shared_ptr<_Tp>::shared_ptr(nullptr_t __p, _Dp __d)
     : __ptr_(0)
 {
 #ifndef _LIBCPP_NO_EXCEPTIONS
-    try
-    {
+    try {
 #endif  // _LIBCPP_NO_EXCEPTIONS
-        typedef typename __shared_ptr_default_allocator<_Tp>::type _AllocT;
-        typedef __shared_ptr_pointer<nullptr_t, _Dp, _AllocT > _CntrlBlk;
-        __cntrl_ = new _CntrlBlk(__p, __d, _AllocT());
+      __cntrl_ = __allocate_shared(__p, __d);
 #ifndef _LIBCPP_NO_EXCEPTIONS
-    }
-    catch (...)
-    {
+    } catch (...) {
         __d(__p);
         throw;
     }
 #endif  // _LIBCPP_NO_EXCEPTIONS
 }
 
 template<class _Tp>
 template<class _Yp, class _Dp, class _Alloc>
 shared_ptr<_Tp>::shared_ptr(_Yp* __p, _Dp __d, _Alloc __a,
                             typename enable_if<is_convertible<_Yp*, element_type*>::value, __nat>::type)
     : __ptr_(__p)
 {
 #ifndef _LIBCPP_NO_EXCEPTIONS
-    try
-    {
+    try {
 #endif  // _LIBCPP_NO_EXCEPTIONS
-        typedef __shared_ptr_pointer<_Yp*, _Dp, _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(__p, __d, __a);
-        __cntrl_ = _VSTD::addressof(*__hold2.release());
-        __enable_weak_this(__p, __p);
+      __cntrl_ = __allocate_shared(__p, __d, __a);
 #ifndef _LIBCPP_NO_EXCEPTIONS
-    }
-    catch (...)
-    {
+    } catch (...) {
         __d(__p);
         throw;
     }
 #endif  // _LIBCPP_NO_EXCEPTIONS
 }
 
 template<class _Tp>
 template<class _Dp, class _Alloc>
 shared_ptr<_Tp>::shared_ptr(nullptr_t __p, _Dp __d, _Alloc __a)
     : __ptr_(0)
 {
 #ifndef _LIBCPP_NO_EXCEPTIONS
-    try
-    {
+    try {
 #endif  // _LIBCPP_NO_EXCEPTIONS
-        typedef __shared_ptr_pointer<nullptr_t, _Dp, _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(__p, __d, __a);
-        __cntrl_ = _VSTD::addressof(*__hold2.release());
+      __cntrl_ = __allocate_shared(__p, __d, __a);
 #ifndef _LIBCPP_NO_EXCEPTIONS
-    }
-    catch (...)
-    {
+    } catch (...) {
         __d(__p);
         throw;
     }
 #endif  // _LIBCPP_NO_EXCEPTIONS
 }
 
 template<class _Tp>
 template<class _Yp>