This patch removes shared_ptr::make_shared as it is not part of the standard. This patch also adds __create_with_cntrl_block, which is a helper function that can be used in std::allocate_shared and std::make_shared. This is the third patch (out of 4) from D66178.
Can you confirm this is the bit of the patch that we suspect had caused a SEGFAULT for @phosek?
Also, is there a reason why we can't just rename shared_ptr<_Tp>::make_shared to shared_ptr<_Tp>::__make_shared, without changing the logic and the signature? Do you have other changes pending that depend on that?
Note: Requesting changes so it shows up properly in my review queue.
@ldionne, yes. I suspect this is the patch that will cause a SEGFAULT. It could also be a similar part of the patch where I remove std::shared_ptr::allocate_shared, though. If it does cause a SEGFAULT we will know exactly where the issue is and it will be easier to debug.
We could mangle the function. But, part of the reason for this change is removing as much code from make_shared as possible so, later, when I add more overloads, there will be less duplication. Additionally, I don't think it makes sense to add an extra layer of abstraction when it gives us no benefit. If we don't want __create_with_cntrl_block I would still like to remove std::shared_ptr::make_shared (and std::shared_ptr::__make_shared).
D62641 depends on this functionality but, it shouldn't be too hard to update that patch (and I wouldn't mind doing it if you want me to remove __create_with_cntrl_block).
Can we call this __create_with_control_block instead? We're not saving many characters.
This just jumped in my face -- it's obvious now.
We're calling a function f(p.get()->foobar(), p.release()) where p is a unique_ptr. If the second argument (p.release()) is evaluated before the first one, p.get() will be nullptr and we'll dereference that, causing the segfault. @phosek must have been testing on a target where the compiler evaluates arguments from right to left.
I think we pinned down the order of evaluation in C++20 (I'm not sure whether it actually got in), but it's definitely a problem for earlier standards anyway.
So, actually, I just got confirmation from @Bigcheese that there was a proposal to make evaluation of arguments be from left to right, but it didn't go through. Instead, the order moved from unsequenced to indeterminately sequenced in C++17.
This means that evaluation of arguments can't be interleaved, but you still can't rely on any given order.