Remove Model::disownAllComponents() method and fix test cases accordingly

[aymanhab]

The method Model::disownAllComponents() was introduced to work around improper memory management when stack allocated objects are added to a Model that takes ownership of them, it potentially introduces leaks (since no components are deallocated on Model destruction) and promotes improper use of stack allocated objects (mostly seen in test cases). Per discussion with @sherm1 and @aseth1 we'll stick to heap allocated Models and ModelComponents (using new) so that we don't have to invoke the above method.

[aseth1]

I labeled this as a FeatureRequest although it is related to a bigger issue of memory management. The latest changes to the Component has not addressed the issue head on. There is nothing stopping users to stack allocate Components, so even if we make all the examples (which we have) and all the tests, it still will be possible and people will do it.

I thought about always cloning components added by reference, but then users still have a handle to the original component and are making changes that have no effect. I am open to all solutions. It is worth doing right.

[sherm1]

This might be a great time to switch to smart pointers, but we would need to experiment. std::unique_ptr has the right semantics for sub-objects that have to have unique owners. Ownership can be transferred but not shared. We would have to see what this does to wrapping; it could be a problem.

The "perfect" method is to create handle objects and not use pointers at all, as Simbody does for most of its important classes. However, I can attest to that being quite a lot of extra work and complexity because every object gets split into at least two classes. Most C++ APIs don't go to that much trouble and if I were starting over I probably wouldn't either. The more common convention is to require users to use new to create subobjects and always transfer ownership to their parent objects. The std::unique_ptr and std::shared_ptr in C++ 11 were designed to make this style clearer and safer and don't require any extra work.

[chrisdembia]

I would love switching to smart pointers, and would be happy to do any grunt work to support the conversion. In CS 249a, the professor always had us add a typedef std::unique_ptr<ClassName> Ptr; to each class so people could use the pointer object as ClassName::Ptr, which is a lot cleaner than having to type out unique_ptr each time. This would be easy for us to do, given the macros we have.

What's the benefit of using unique_ptr over shared_ptr?

[sherm1]

What's the benefit of using unique_ptr over shared_ptr?

unique_ptr is overhead-free and has very simple semantics. shared_ptr is more complicated, to the point where sometimes you have to use an std::weak_ptr to avoid leaks that will occur, for example, in a circular linked list of shared_ptrs. Stroustrup has a discussion of this C++ 4th ed. pg 991. To summarize he says, when there is a choice:

• prefer unique_ptr to shared_ptr
• prefer ordinary scoped objects to objects on the heap owned by unique_ptr

Also, unique/shared/weak don't quite capture all the cases. We have ClonePtr, which is like a copyable unique_ptr, and ReferencePtr which is like a very weak pointer that doesn't affect ownership at all and zeroes itself when copied. These latter types should never appear in the API but are very useful internally.

[sherm1]

add a typedef std::unique_ptr Ptr; to each class so people could use the pointer object as ClassName::Ptr, which is a lot cleaner than having to type out unique_ptr each time

This doesn't seem like a good idea to me because it presumes there is only one kind of meaningful pointer. One could need plain, unique or shared pointer access to the same kind of object under different circumstances. We could add typedef std::unique_ptr<ClassName> UniquePtr, etc. to each class but then it isn't any easier than using std::unique_ptr or shared_ptr directly.