Hi There has been a long thread in the dev list discussing about whether should we use unique_ptr or not in our code base. Instead of arguing at a high level, let us see some examples. I've got a very good example.
There is a bug currently in the master branch (Unfortunately, I introduced the bug:() https://github.com/apache/mesos/blob/master/include/mesos/resources.hpp#L235 The bug is introduced in this patch (https://reviews.apache.org/r/29018). For those of you who do not have the context, we want to introduce a concept called Resources transformation. We also follow the composite design pattern <http://en.wikipedia.org/wiki/Composite_pattern>here: class Transformation { virtual Try<Resources> apply(const Resources& resources) const = 0; } class CompositeTransformation : public Transformation { virtual Try<Resources> apply(const Resources& resources) const {...} template <typename T> void add(const T& t) { transformations.push_back(new T(t)); } std::vector<Transformation*> transformations; } Now the bug is that we haven't deleted the copy constructor for CompositeTransformation which could result in memory corruption (double free). That's a very good example showing that using raw pointers is dangerous. You could accidentally introduce some bug that you are not aware of. Well, people might say that you should delete the copy constructor. Turns out that if we delete the copy constructor for CompositeTransformation, the 'add' method won't work if we want to add a CompositeTransformation to a CompositeTransformation (since we introduced a copy in 'add'). Should we use Owned here? Turns out that it's also not desired because include/mesos/resources.hpp is a mesos header. If we introduce Owned here, we implicitly introduce a dependency between scheduler and libprocess. My suggestion for the fix is to use unique_ptr (std::vector<unique_ptr<Transformation*>> transformations). By using unique_ptr, the copy constructor will be deleted by default. Although, another options is to manually implement the copy constructor for CompositeTransformation. Obviously, having both unique_ptr and Owned in the code base is also not desired. That's reason I suggested in the previous email that we should kill Owned and allow unique_ptr <-> process::Shared transformation. In our async actor based programming model, we tend to use copy a lot because it fits well in this model (programmers do not need to reason about the ownership). However, the copying model does not work well with polymorphism and we don't have many polymorphism in our code base. I guess that's the main reason we haven't decided on this style issue yet. Thoughts? - Jie
