[libcxx-commits] [PATCH] D62233: General shared_ptr cleanup

[Louis Dionne via Phabricator via libcxx-commits]

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>
-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
-
-#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
  https://reviews.llvm.org/D62233/new/

https://reviews.llvm.org/D62233