[libcxx-commits] [PATCH] D62233: General shared_ptr cleanup
Zoe Carver via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Aug 13 14:46:22 PDT 2019
zoecarver marked 4 inline comments as done.
zoecarver added a comment.
That is a completely valid concern. I will try to focus this patch more.
Comment at: include/memory:3663
+ virtual void __on_zero_shared() _NOEXCEPT;
+ virtual void __on_zero_shared_weak() _NOEXCEPT;
> I don't see the point in moving those functions around. I would leave them where they were to reduce the diff unless there's a reason to move them around (in which case please just let me know).
Just for readability. We used to have:
Now it is:
I will revert.
Comment at: include/memory:3808
+ !is_array<_Yp>::value &&
+ is_convertible<typename unique_ptr<_Yp, _Dp>::pointer,
> Since you're deducing `_Dp&` now, this needs to be `is_convertible<typename unique_ptr<_Yp, _Dp&>::pointer`.
> I *think* the deduction like what you wrote works, however the fact that we've been using `is_lvalue_reference<_Dp>` previously makes me suspicious that I'm missing something subtle. If the tests pass, though, I would say this is OK.
Good catch. I will test with `_LIBCPP_HAS_NO_RVALUE_REFERENCES`.
Comment at: include/memory:4322
> I'd be more comfortable if you inlined the `std::shared_ptr::make_shared`s verbatim into their respective callers (`std::make_shared`). But that would require friendship, and that's the purpose of `std::shared_ptr::__create_with_cntrl_block` being a static member function, right?
Yes. While having these functions be private would be better than what we currently have, I would much prefer not to use friendship and keep all this duplicate code. The main purpose of `__create_with_cntrl_block ` is to remove duplicate code. Going forward it will be much easier to update all these functions at once (though, hopefully, we will remove these variadics soon).
Comment at: include/memory:4358
-#else // _LIBCPP_HAS_NO_VARIADICS
> There's a lot of stuff going on in this patch. The removal of variadics below could be split off in a separate patch, that's what I meant in my previous comment.
I am happy to move `__create_with_cntrl_block` and all changes to these functions into another patch. Just say the word. I want to confirm we are the same page first.
CHANGES SINCE LAST ACTION
More information about the libcxx-commits