Cleanup extra memory allocation, and port boost to std shared_ptr

[0xFFFC0000]

a) Use make_shared which removes one extra allocation [1].
b) Port from boost::shared_ptr to std::shared_ptr.
c) use = default syntax instead of {}. Which is cleaner.

1. https://stackoverflow.com/a/20895705

[hyc]

Be sure to test this on Windows before accepting the PR. Windows' support for std:: features was broken in previous versions of g++.

[0xFFFC0000]

@hyc thanks. Absolutely. I will double check, and will give update here.

[vtnerd]

I didn't even know =default could be combined with virtual. I don't see any advantages (or disadvantages) - it should be identical in behavior?

[0xFFFC0000]

In addition to being more readable, it would prevent a subtle bug:

https://stackoverflow.com/a/827205

[vtnerd]

There is no bug in the existing code related to derived destructors, that post is incorrect. Its suggesting that you couldn't do derived class destruction before C++11, which is false.

[0xFFFC0000]

Thanks for the explanation.

Keep in mind I didn't say it does have a bug. I said it can cause a very subtle bug in very very rare cases, because of differences in behaviours. And that link while having some incorrect information, was just to show there are very subtle differences between the two approaches [1].

1. https://godbolt.org/z/x7odMqb37

[vtnerd]

While we're making this change - the variable slc can be removed and f should be moved (although I don't think call_befre_die properly moves either).

[0xFFFC0000]

The compiler should take care of that. The reason why we need that optimization is because there is an extra memory allocation here. Once we remove the extra allocation, the compiler should take care of move optimization. In this somewhat similar example [1], the assembly code generated for 1 is different than 2 and 3.

1. https://godbolt.org/z/bo4d5eMKd

[vtnerd]

I realize the compiler will (mostly likely) do the same thing for the named variable, but why even have it?

And your callback example is too trivial to make a difference - try with a non-trivial destructor in the callback. I doubt the compiler can "assume" its a move unless specifically told to do so.

[vtnerd]

Add this type to the member list:

struct complex
{
    complex(const complex&);
    complex(complex&&);
    ~complex();
};

and the copy-constructor is called, unless TWO std::moves are added. One here, and the other in call_before_die. We might as well add it in both spots, I don't see an advantage to always calling the copy-constructor.

[0xFFFC0000]

The compiler is able to apply return-value-optimization even in that case too [1, 2].

As you can see from the diff page in [1, 2], both versions, even for complex types are identical. (Take a look at RVO in [3]).

1. https://godbolt.org/z/7Pr3f4xdf
2. https://godbolt.org/z/1hTKMd9Kc
3. https://sigcpp.github.io/2020/06/08/return-value-optimization

[0xFFFC0000]

Now that being said, I don't have any issue with changing the code to :

  return std::make_shared<call_befor_die<t_scope_leave_handler>>(f);

My point is it is a matter of cosmetic. So if you think that is better, let me know, and I will change it to that.

[vtnerd]

I know the compiler will almost certainly apply NVRO, which is why I said: "will (mostly likely) do the same thing". That was in reference to the named value in the return. It is purely cosmetic, and I would've just dropped it since it useless. Keep it for historical reasons if you want.

The other comment was about std::move. I should've started a separate issue for that, to avoid confusion. Your example isn't highlighting the problem easily, because copy and moves are similar in your example. Remove the definition (leaving just the declaration), and you can easily see that the copy constructor is being called twice for this function, when it could be calling the move constructor twice. In some cases the two are identical, but not in all cases.

[0xFFFC0000]

The other comment was about std::move. I should've started a separate issue for that, to avoid confusion. Your example isn't highlighting the problem easily, because copy and moves are similar in your example. Remove the definition (leaving just the declaration), and you can easily see that the copy constructor is being called twice for this function, when it could be calling the move constructor twice. In some cases the two are identical, but not in all cases.

I see what you saying, but that is totally different issue than what I am trying to solve in this PR. My main goal is to remove the extra memory allocation happening here. There are a lot of areas that std::move can help with optimization. But that does not fall into this PR area.

By the way, no need to remove the definition. Adding simple __attribute__ ((noinline)) would show the copy constructor gets called in both ways. So we can say not directly related to this PR anyway.

1. https://godbolt.org/z/P9nrbzoPc

[vtnerd]

Be sure to test this on Windows before accepting the PR. Windows' support for std:: features was broken in previous versions of g++.

std::shared_ptr is already used elsewhere, so it should be fine.

[0xFFFC0000]

Thanks @vtnerd for confirming std::shared_ptr on Windows.

[0xFFFC0000]

Putting the compiler explorer example [1] here too. In case anybody is interested.

1. https://godbolt.org/z/bo4d5eMKd

[0xFFFC0000]

@hyc @vtnerd I know you guys busy. But appreciate if you take another look and please let me know if you think any other changes necessary for this PR. Thanks.