Skip to content

Conversation

@ldionne
Copy link
Member

@ldionne ldionne commented Oct 23, 2024

Since we can use if constexpr in all standard modes (it is supported as a compiler extension), we can simplify the implementation of __shared_ptr_emplace.

…_ptr

Since we can use `if constexpr` in all standard modes (it is supported
as a compiler extension), we can simplify the implementation of
__shared_ptr_emplace.
@ldionne ldionne requested a review from a team as a code owner October 23, 2024 21:17
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Oct 23, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 23, 2024

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

Since we can use if constexpr in all standard modes (it is supported as a compiler extension), we can simplify the implementation of __shared_ptr_emplace.


Full diff: https://github.com/llvm/llvm-project/pull/113495.diff

1 Files Affected:

  • (modified) libcxx/include/__memory/shared_ptr.h (+18-29)
diff --git a/libcxx/include/__memory/shared_ptr.h b/libcxx/include/__memory/shared_ptr.h
index 65870ba574c25b..080ced077b7338 100644
--- a/libcxx/include/__memory/shared_ptr.h
+++ b/libcxx/include/__memory/shared_ptr.h
@@ -252,22 +252,17 @@ template <class _Tp, class _Alloc>
 struct __shared_ptr_emplace : __shared_weak_count {
   using __value_type = __remove_cv_t<_Tp>;
 
-  template <class... _Args,
-            class _Allocator                                                                         = _Alloc,
-            __enable_if_t<is_same<typename _Allocator::value_type, __for_overwrite_tag>::value, int> = 0>
+  template <class... _Args>
   _LIBCPP_HIDE_FROM_ABI explicit __shared_ptr_emplace(_Alloc __a, _Args&&...) : __storage_(std::move(__a)) {
-    static_assert(
-        sizeof...(_Args) == 0, "No argument should be provided to the control block when using _for_overwrite");
-    ::new (static_cast<void*>(__get_elem())) __value_type;
-  }
-
-  template <class... _Args,
-            class _Allocator                                                                          = _Alloc,
-            __enable_if_t<!is_same<typename _Allocator::value_type, __for_overwrite_tag>::value, int> = 0>
-  _LIBCPP_HIDE_FROM_ABI explicit __shared_ptr_emplace(_Alloc __a, _Args&&... __args) : __storage_(std::move(__a)) {
-    using _TpAlloc = typename __allocator_traits_rebind<_Alloc, __value_type>::type;
-    _TpAlloc __tmp(*__get_alloc());
-    allocator_traits<_TpAlloc>::construct(__tmp, __get_elem(), std::forward<_Args>(__args)...);
+    if constexpr (is_same<typename _Alloc::value_type, __for_overwrite_tag>::value) {
+      static_assert(
+          sizeof...(_Args) == 0, "No argument should be provided to the control block when using _for_overwrite");
+      ::new (static_cast<void*>(__get_elem())) __value_type;
+    } else {
+      using _TpAlloc = typename __allocator_traits_rebind<_Alloc, __value_type>::type;
+      _TpAlloc __tmp(*__get_alloc());
+      allocator_traits<_TpAlloc>::construct(__tmp, __get_elem(), std::forward<_Args>(__args)...);
+    }
   }
 
   _LIBCPP_HIDE_FROM_ABI _Alloc* __get_alloc() _NOEXCEPT { return __storage_.__get_alloc(); }
@@ -275,22 +270,16 @@ struct __shared_ptr_emplace : __shared_weak_count {
   _LIBCPP_HIDE_FROM_ABI __value_type* __get_elem() _NOEXCEPT { return __storage_.__get_elem(); }
 
 private:
-  template <class _Allocator                                                                         = _Alloc,
-            __enable_if_t<is_same<typename _Allocator::value_type, __for_overwrite_tag>::value, int> = 0>
-  _LIBCPP_HIDE_FROM_ABI void __on_zero_shared_impl() _NOEXCEPT {
-    __get_elem()->~__value_type();
-  }
-
-  template <class _Allocator                                                                          = _Alloc,
-            __enable_if_t<!is_same<typename _Allocator::value_type, __for_overwrite_tag>::value, int> = 0>
-  _LIBCPP_HIDE_FROM_ABI void __on_zero_shared_impl() _NOEXCEPT {
-    using _TpAlloc = typename __allocator_traits_rebind<_Allocator, __remove_cv_t<_Tp> >::type;
-    _TpAlloc __tmp(*__get_alloc());
-    allocator_traits<_TpAlloc>::destroy(__tmp, __get_elem());
+  _LIBCPP_HIDE_FROM_ABI_VIRTUAL void __on_zero_shared() _NOEXCEPT override {
+    if constexpr (is_same<typename _Alloc::value_type, __for_overwrite_tag>::value) {
+      __get_elem()->~__value_type();
+    } else {
+      using _TpAlloc = typename __allocator_traits_rebind<_Alloc, __remove_cv_t<_Tp> >::type;
+      _TpAlloc __tmp(*__get_alloc());
+      allocator_traits<_TpAlloc>::destroy(__tmp, __get_elem());
+    }
   }
 
-  _LIBCPP_HIDE_FROM_ABI_VIRTUAL void __on_zero_shared() _NOEXCEPT override { __on_zero_shared_impl(); }
-
   _LIBCPP_HIDE_FROM_ABI_VIRTUAL void __on_zero_shared_weak() _NOEXCEPT override {
     using _ControlBlockAlloc   = typename __allocator_traits_rebind<_Alloc, __shared_ptr_emplace>::type;
     using _ControlBlockPointer = typename allocator_traits<_ControlBlockAlloc>::pointer;

@ldionne
Copy link
Member Author

ldionne commented Oct 24, 2024

@philnik777 This doesn't seem to work in C++03 mode. I thought you mentioned something along the lines that this should be working in all standard modes. Did I invent that?

@philnik777
Copy link
Contributor

@philnik777 This doesn't seem to work in C++03 mode. I thought you mentioned something along the lines that this should be working in all standard modes. Did I invent that?

Eh, I think you invented that. Anything constexpr doesn't work in C++03 mode. Attributes and lambdas (in new versions of clang) work in C++03 though. FWIW this is one of the simplifications we could do all over the code base when we split C++03 and non-C++03 code.

@ldionne
Copy link
Member Author

ldionne commented Oct 24, 2024

Dropping the change for now since I seem to have invented that it was possible.

@ldionne ldionne closed this Oct 24, 2024
@ldionne ldionne deleted the review/refactor-allocate-shared branch October 24, 2024 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants