Use of shared pointers in public interfaces

Question

We have a pretty standard tree API using shared pointers that looks roughly like this (implementations omitted for brevity):

class node;
using node_ptr = std::shared_ptr<node>;

class node : public std::enable_shared_from_this<node> {
    std::weak_ptr<node> parent;
    std::vector<node_ptr> children;

public:
    virtual ~node() = default;

    virtual void do_something() = 0;

    void add_child(node_ptr new_child);

    void remove_child(node_ptr child);

    node_ptr get_parent();

    const std::vector<node_ptr>& get_children();
};

class derived_node : public node {
    derived_node() = default;

public:
    virtual void do_something() override;

    static node_ptr create(/* args... */);
};

// More derived node types...

This works just fine and prevents nodes being leaked as you'd imagine. However, I've read on various other answers on SO that using std::shared_ptr in a public API like this is considered bad style and should be avoided.

Obviously this ventures into opinion-based territory, so a couple of concrete questions to avoid this question being closed :-)

• Are there any well-known pitfalls to using shared_ptrs in interfaces like this, which we have so far been fortunate enough to avoid?

• If so, is there a commonly-used (I hesitate to say "idiomatic") alternative formulation which avoids said pitfalls but still allows for simple memory management for users?

Thanks.

Answer 1

std::shared_ptr is to manage ownership,

so prefer for print_tree function

void print_tree(const node& root); // no owner ship transfer

than

void print_tree(const std::shared_ptr<node>& root);

The later requires a shared_ptr, so may require a construction of a shared_ptr from the object. (whereas retrieving object from shared_ptr is a simple getter)

Now, for your getters, you have mainly the choice between

• share_ptr, if you want to share ownership with user
• weak_ptr, secure reference to internal
• pointer/reference, insecure reference to internal.

By secure and insecure, I mean that if object is destroyed, you may test that with weak_ptr, but not with simple pointer. The security has some overhead though, so there is a trade-off.

If accessors are for local usage only and not to keep reference on it, pointer/reference may be a good option. As example, std::vector::iterator are unusable once the vector is destroyed, so there are good for local usage but may be dangerous to keep iterator as reference (but possible).

Do you expect/allow that user keeps reference of a node but allow the root (or parent) to be destroyed ? what should happen to that node for user ?

For void add_child(node_ptr new_child);, you clearly take ownership. You may hide the shared_ptr if the node construct it child itself, something like

template <typename NodeType, typename...Ts>
std::weak_ptr<NodeType> add_child(Ts&&... args)
{
    auto n = std::make_shared<NodeType>(std::forward<Ts>(args)...);
    // or
    // auto n = NodeType::create(std::forward<Ts>(args)...);
    // Stuff as set parent and add to children vector
    return n;
}

Answer 2

Its not bad style, it depends on your goals, and assumptions.

A few projects I've worked on with hard restraints required us to avoid shared_ptrs because we wanted to manage our own memory. So use of 3rd party libs that would require to use shared_ptrs are out.

Another reason you might wish to avoid shared_ptrs is that its somewhat opinionated. Some projects will wrap everything around it and just pretend its like having a GC language (Urg!). Other projects will treat shared_ptrs with a little more restraint, and only use shared_ptr's when it comes down to actually things that have shared ownership.

Most of the 3rd party API (certainly not all) I've worked with operate on the principle if you've allocated it, you destroy it. So long as your very clear about the ownership of the resource it doesn't cause too much issue.

Answer 3

One reason this may be considered bad style may be the additional overhead involved in reference counting that is often (never say never) not actually needed in external APIs, since they often fall into one of two categories:

1. An API that receives a pointer, acts on it and returns it - a raw pointer will usually work better, since the function does not need to manage the pointer itself in any way
2. An API that manages the pointer, such as in your case - a std::unique_ptr will usually be a better fit, and it has 0 overhead.