Additional guidelines for use of shared_ptr

[MikeGitb]

In short, I wonder if there should be a note in the CONTRIBUTING.md file that cautions against unnecessary use of std::shared_ptr and encourage the use of std::make_shared when a shared_ptr is really necessary.

So far, I've only had a serious look at a very small part of the code base, so this might be a misconception on my part, but I get the impression that std::shared_ptr is somewhat overused.

I'm not talking about the practice to wrap almost any object that is passed around into a shared_ptr. I see the advantage that it gives in terms of simplicity, even, if it might not always be strictly necessary and adds some overhead. However, there seem to be also quite a number of usages, where nothing is shared or passed around and a std::unique_ptr, a std::vector or even just a simple member variable would have been the much better option. An extreme case can be seen here, where std::vector was essentially reimplemented using a shared_ptr - just that shared_ptr to an array with a custom deallocator is an extremely inefficient way to do this due to additional dynamic memory allocation, type erasure and threadsafe reference counting.

Admittedly this could be a problem of the past. The example I mentioned above was from 2010, when std::shared_ptr wasn't even standardized.

[ryanbartley]

The use of shared_ptr in Cinder has been discussed at length, here for instance, as well as on the older forum. Also, I'd say that for posts like these, they'd be better served as forum discussions rather than issues.

[MikeGitb]

@ryanbartley: As I said, I'm not talking about the general pattern to wrap objects into shared pointers - I'm fine with that. But about situations like the case mentioned above, where a shared pointer is used in an awkward way for a local temporary array, which makes the code both slower and more complex.
But you are probably right that this should better be discussed in the forum.

[andrewfb]

I'd say overall that's a fair critique, though the example you're citing is probably unusually egregious, and is (as you said) quite an old piece of the codebase. In fact that code is older than a git blame would imply, as it even antedates Cinder's initial commit in 2010. This is the nature of many hands touching a large codebase over many years though, so commits to update crustier portions like this (or the SurfaceCache cleanup you've been working on) are very welcome. If you're asking about whether there's a formal overuse or misuse shared_ptr's policy, the answer is no :).

A trickier subject is the extent to which shared_ptr is relied upon in interface, rather than in implementation (per the previous two examples). Cinder is used by developers of varying levels of comfort with C++, and often some of the most interesting work comes from those who are not yet experts in the language. There is a balancing act here, and in general shared_ptr has served us well in terms of minimizing the class of bugs it was designed to address. I think move semantics may open some opportunities to reduce our reliance on this occasionally heavy-handed solution in the future. As you stated in your post though, I don't think that's the emphasis of your question.

All that to say, if you see places this kind of thing could be improved, we're generally amenable and grateful for the contribution.

[MikeGitb]

Seems like I've overstated / poorly explained my goal. My main question is:

Should there be two more points in the section about shared_ptr in CONTRIBUTING.md?

• One advocating the use of std::make_shared, which is imho easier to read and avoids a second dynamic memory allocation compared to FooRef( new Foo( ... ).
• A second cautioning (not banning or anything) against the use of shared pointer for local (maybe also private) objects that aren't passed around - in particular c-style arrays.

I got the impression that adding those points (the second one would need more work) might be beneficial based on seeing a few cases where those guidelines are being violated (the example was indeed an exceptional extreme case). However, if those cases are in general very rare or only found in old code, then they would only bloat the style guide without adding real value. Also, if you think they would discourage any potential contributors / make life harder for them I agree they would certainly do more harm than good as it is mainly a efficiency / style matter not a correctness issue.

[richardeakin]

My 2 cents: For code we write today, I think we try to use unique_ptr instead of shared_ptr wherever possible as it is clearer how the data / object is meant to be used. Avoiding explicit calls to new or delete is ideal, and instead we try to use auto pointers with custom wrappers, or something else that can auto-manage the cleanup via destructor. I'm personally a big fan of using std::vector<char> for a raw data buffer, or vector<whatever>, even if you're just going to access a raw pointer to the buffer via .data(). These types of things are constantly in discussion amongst the contributors and as we gravitate towards a consensus, I think it is ideal that we'd get it into the CONTRIBUTING.md guide to make it easier for others less familiar with our codebase to contribute.

So, I'd say both of your points are very good guidelines, and also things we should look to refactoring within the codebase as time permits.

[MikeGitb]

Seems there is not enought support for / interest in this here, so I'm closing this for now.