[libcxx-commits] [PATCH] D62233: General shared_ptr cleanup
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Aug 13 13:58:46 PDT 2019
ldionne added a comment.
This patch touches a lot of code, and the code it touches is very sensitive (`std::shared_ptr` is used literally everywhere). That's why I like being able to easily verify its correctness, which reducing the diff or just moving code around (without modifying it -- as much as possible) will make easier in this case.
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).
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.
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?
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.
CHANGES SINCE LAST ACTION
More information about the libcxx-commits