Using smart pointers instead of raw pointers

[jacksondunstan]

@JmgrArt, could you please explain why this change is beneficial?

[JmgrArt]

C++11 introduced a set of smart pointers that allows developer to manage pointers in a safe and yet simple way. Contrary to raw pointers, they help keeping exception safety and prevent users from doing dangerous things like forgetting to delete a pointer after use or deleting a pointer still used within another part of the application. Generally speaking, the use of raw pointers in modern C++ is discouraged, and one should never have to use the "new" or "delete" keywords. This change would promote the use of standard smart pointers instead of raw pointers (even if in this case there was no danger of exception safety or other things like that ; it could help future developments using this code and reduce bug occurrence).

[jacksondunstan]

Hi @JmgrArt, thanks for the pull request and for providing the requested explanation. I understand the general case for "smart pointers" over direct usage of new and delete. In the case of this project and the Plugin::unhandledCsharpException variable specifically that you've modified, I think smart pointers add more confusion than they clear up. The reader must now understand std::move, which entails an understanding of lvalue references, rvalue references, and xvalue references. Many readers will not grasp the subtleties here, but most will understand the relative simplicity of new and delete.

Further, the variable is no longer deleted at the std::move point but instead persists in memory until the pointer is overwritten. Here's a quick example replicating the proposed change in a small way. Note that "dtor" is not printed in the output. While this can surely be remedied, it illustrates that "smart pointers" aren't helping us avoid bugs in this case.

In general, this project has taken a very manual approach to memory management. It makes heavy use of free lists and handles in both C# and C++ as well as "placement new" for manual memory allocation. "Smart pointers" surely have their place, but they're a bit of a mismatch for the low-level nature of the bindings code in this project.

Finally, the project currently has no dependency on the C++ Standard Library (a.k.a. STL) and using "smart pointers" would add that dependency. So for these reasons I think it's best to stick with plain new and delete for now. However, this pull request has pointed out a bug for me! The delete is never being called because the previous line calls ThrowReferenceToThis which throws an exception, thus skipping the delete line. I'll fix this in an upcoming commit. Thanks!

[JmgrArt]

You don't create any instance of S in your example, but I get your point. I could reproduce this behavior with a few changes. I assumed that moving a pointer was enough to limit its lifetime to the current context, which is false.