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

[Zoe Carver via Phabricator via libcxx-commits]

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;
----------------
ldionne wrote:
> 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:
```
// code
public:
//code
private:
//code
public:
//code
```
Now it is:
```
private:
//code
public:
//code
```
I will revert. 


================
Comment at: include/memory:3808
+                !is_array<_Yp>::value &&
+                is_convertible<typename unique_ptr<_Yp, _Dp>::pointer,
+                                    element_type*>::value,
----------------
ldionne wrote:
> 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
-shared_ptr<_Tp>
-shared_ptr<_Tp>::make_shared(_Args&& ...__args)
-{
----------------
ldionne wrote:
> 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
-
----------------
ldionne wrote:
> 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
  https://reviews.llvm.org/D62233/new/

https://reviews.llvm.org/D62233