[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,
+                                    element_type*>::value,
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
-shared_ptr<_Tp>::make_shared(_Args&& ...__args)
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
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.



More information about the libcxx-commits mailing list