Page MenuHomePhabricator

General shared_ptr cleanup
AbandonedPublic

Authored by zoecarver on May 21 2019, 10:04 PM.

Details

Summary

This patch applies some general cleanup to shared_ptr and its implementation structures:

  • stop using compressed_pair to store objects.
  • implement __allocate_shared to reduce duplicate code.
  • remove _LIBCPP_HAS_NO_VARIADICS. I don't think it is necessary, but I can add it back if it is.

Diff Detail

Event Timeline

zoecarver created this revision.May 21 2019, 10:04 PM
EricWF added inline comments.May 21 2019, 10:31 PM
include/memory
3668

Changing the __compressed_pair to these two separate members changes the layout of __shared_ptr_emplace and in turn breaks the ABI.

4040

You're deleting a nullptr here. IDK if that makes sense.

stop using compressed_pair to store objects.

Besides breaking the ABI, what's the motivation to stop using compressed_pair?
Many allocators are "state-free", and there's no need to actually *store* one of them.
That's the purpose of compressed_pair, to not store objects that have no state.

given two types T and A, where A has no non-static data members (and no virtual functions), we have

sizeof(std::pair<T, A>) == sizeof(T) + sizeof(A) + possible padding.
sizeof(compressed_pair<T,A>) == sizeof(T)

Thanks for the explanation @mclow.lists I didn't fully understand the importance of using compressed_pair (or that removing it would cause an ABI break). I don't have a "good" reason for removing compressed_pair other than, I got tired of typing out .second().second() and remembering which child class uses which order of elements (which is not a good reason). I will remove that change.

zoecarver updated this revision to Diff 200758.May 22 2019, 8:26 AM
  • remove changes to control block around compressed_pair

remove _LIBCPP_HAS_NO_VARIADICS. I don't think it is necessary, but I can add it back if it is.

Are you running the tests with -std=c++03?, because that's where _LIBCPP_HAS_NO_VARIADICS gets set.

I thought shared_ptr was not introduced until C++11?

I thought shared_ptr was not introduced until C++11?

Libc++ is a c++11 library; it provides many c++11 features when compiling for C++03.

zoecarver updated this revision to Diff 200866.May 22 2019, 7:55 PM
  • re-add functions for _LIBCPP_HAS_NO_VARIADICS
zoecarver updated this revision to Diff 201453.May 26 2019, 9:09 AM
  • remove static member functions shared_ptr::make_shared and shared_ptr::allocate_shared (and move their contents to the correct place).
zoecarver updated this revision to Diff 201456.May 26 2019, 9:13 AM
  • fix typedef order (sorry for repetitive updates).
  • fix, so it works in c++03
zoecarver marked an inline comment as done.May 31 2019, 6:42 PM
zoecarver added inline comments.
include/memory
4040

If it matters, I can change it, but I don't think it should. If, for example, a shared_ptr to a nullptr went out of scope, the same thing would happen, so people's deleters should be able to handle this (especially if they are ever passing it a nullptr).

Friendly ping @mclow.lists @EricWF and @ldionne. I would very much like to get this committed so that I can update my following patches. I know @EricWF has some patches that may conflict with this, I am happy to update based on those or help update those based on this. Regardless, getting this committed is a very high priority for me.

ldionne requested changes to this revision.Aug 13 2019, 7:29 AM

Could you please split this patch into one that removes the variadic stuff, and one that does the rest (so we can look at them separately). I think we can get rid of the variadic stuff because of Clang extensions, but it's not clear to me the rest is worth doing.

This revision now requires changes to proceed.Aug 13 2019, 7:29 AM

Could you please split this patch into one that removes the variadic stuff, and one that does the rest (so we can look at them separately). I think we can get rid of the variadic stuff because of Clang extensions, but it's not clear to me the rest is worth doing.

@EricWF is going to remove the variadics here. The code I removed here is removing shared_ptr::make_shared and shared_ptr::allocate_shared. Those two static member functions should not exist and are not part of the standard. Would it be helpful for me to break this patch down further into smaller patches?

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.

include/memory
3663

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).

3808

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.

4322

I'd be more comfortable if you inlined the std::shared_ptr::make_shareds 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?

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.

zoecarver marked 4 inline comments as done.Aug 13 2019, 2:43 PM

That is a completely valid concern. I will try to focus this patch more.

include/memory
3663

Just for readability. We used to have:

// code
public:
//code
private:
//code
public:
//code

Now it is:

private:
//code
public:
//code

I will revert.

3808

Good catch. I will test with _LIBCPP_HAS_NO_RVALUE_REFERENCES.

4322

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).

4358

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.

zoecarver abandoned this revision.Aug 13 2019, 3:36 PM

I am closing this patch in favor of D66177 and D66178. I may submit another patch for the remaining changes, but that is a much lower priority. I am leaving this patch intact for posterity.