[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

This revision was automatically updated to reflect the committed changes. Closed by commit rL303874: Add support for shared_ptr (authored by epilk). Changed prior to commit: https://reviews.llvm.org/D30837?vs=97313=100251#toc Repository: rL LLVM https://reviews.llvm.org/D30837 Files:

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

EricWF accepted this revision. EricWF added a comment. This revision is now accepted and ready to land. LGTM after addressing inline comments. Sorry for the delay. Comment at: include/memory:3663 +template +struct rebind +{ This rebind isn't

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

erik.pilkington added a comment. Ping! @mclow.lists: Do you have any thoughts here? https://reviews.llvm.org/D30837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

erik.pilkington updated this revision to Diff 97313. erik.pilkington added a comment. Rebase n' ping! https://reviews.llvm.org/D30837 Files: include/memory test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

erik.pilkington added inline comments. Comment at: include/memory:3606 +template <> +class _LIBCPP_TEMPLATE_VIS allocator<__shared_ptr_dummy_rebind_allocator_type> +{ EricWF wrote: > I would prefer using an entirely different allocator type, not a >

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

erik.pilkington updated this revision to Diff 95210. erik.pilkington added a comment. This new patch includes @EricWF's static_assert & test. Thanks, Erik https://reviews.llvm.org/D30837 Files: include/memory

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

EricWF added a comment. - Please apply this patch on top of yours. It adds tests for the diagnostics produced when the default deleter is used. LGTM other than the inline comments. I'll want to run this by @mclow.lists before

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

erik.pilkington added a comment. Ping! https://reviews.llvm.org/D30837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

erik.pilkington updated this revision to Diff 93488. erik.pilkington marked 2 inline comments as done. erik.pilkington added a comment. In this new patch, use an explicit specialization of std::allocator that specifically only performs a rebind. This needs to be a specialization of

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

EricWF added inline comments. Comment at: include/memory:3933 +typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, _AllocT > _CntrlBlk; +__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), _AllocT()); __hold.release(); Quuxplusone wrote: >

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

Quuxplusone added inline comments. Comment at: include/memory:3933 +typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, _AllocT > _CntrlBlk; +__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), _AllocT()); __hold.release(); EricWF wrote: >

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

EricWF added a comment. This patch seems to support constructing a `shared_ptr` without providing a non-default deleter. I don't think this should work because the default deleter will attempt to free a function pointer, which is never valid. (Although I think this case will still cause a

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

erik.pilkington added a comment. Ping! https://reviews.llvm.org/D30837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

erik.pilkington added a comment. In https://reviews.llvm.org/D30837#698305, @EricWF wrote: > We can't just use an arbitrary allocator type for a number of reasons: > > - You just changed the type of the control block. That's ABI breaking. Ah, I didn't think of that. This new patch only selects

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

erik.pilkington updated this revision to Diff 91660. erik.pilkington added a comment. In this new patch: - We only select `allocator` when _Yp is a function type, fixing the ABI break @EricWF pointed out. - Only try to select a different allocator if we're using the `__shared_ptr_pointer`

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

EricWF requested changes to this revision. EricWF added a comment. This revision now requires changes to proceed. We can't just use an arbitrary allocator type for a number of reasons: - You just changed the type of the control block. That's ABI breaking. - `allocator` allocates ints, nothing

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

Quuxplusone added a comment. I notice that the Standard implies that std::unique_ptr will work only with *object types* T, and yet the "object" wording is missing from shared_ptr. > A unique pointer is an object that owns another *object* and manages that > other *object* through a pointer.

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

erik.pilkington updated this revision to Diff 91385. erik.pilkington added a comment. This new patch replaces the allocator from `allocator` to `allocator`, I didn't realize `allocator` was deprecated. Thanks, Erik https://reviews.llvm.org/D30837 Files: include/memory

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

halyavin added a comment. But std::allocator is deprecated in C++17. I don't know a good solution, I just used int as an arbitrary type when I faced similar problem. https://reviews.llvm.org/D30837 ___ cfe-commits mailing list

[PATCH] D30837: [libcxx] Support for shared_ptr<T()>

erik.pilkington created this revision. This patch adds support for `shared_ptr` types, so that the following works: void Func(); void Del(void (*)()) std::shared_ptr x(Func, Del); Where previously this would fail to compile. In PR27566, the use case described for this was a