Idiomatic use of auto_ptr to transfer ownership to a container

I'm refreshing my C++ knowledge after not having used it in anger for a number of years. In writing some code to implement some data structure for practice, I wanted to make sure that my code was exception safe. So I've tried to use std::auto_ptrs in what I think is an appropriate way. Simplifying somewhat, this is what I have:

class Tree
{
public:
    ~Tree() { /* delete all Node*s in the tree */ }
    void insert(const string& to_insert);
    ...

private:
    struct Node {
        ...
        vector<Node*> m_children;
    };

    Node* m_root;
};

template<T>
void push_back(vector<T*>& v, auto_ptr<T> x)
{
    v.push_back(x.get());
    x.release();
}

void Tree::insert(const string& to_insert)
{
    Node* n = ...;  // find where to insert the new node
    ...
    push_back(n->m_children, auto_ptr<Node>(new Node(to_insert));
    ...
}

So I'm wrapping the function that would put the point开发者_JAVA百科er into the container, vector::push_back, and relying on the by-value auto_ptr argument to ensure that the Node* is deleted if the vector resize fails.

Is this an idiomatic use of auto_ptr to save a bit of boilerplate in my Tree::insert? Any improvements you can suggest? Otherwise I'd have to have something like:

Node* n = ...;  // find where to insert the new node
auto_ptr<Node> new_node(new Node(to_insert));
n->m_children.push_back(new_node.get());
new_node.release();

which kind of clutters up what would have been a single line of code if I wasn't worrying about exception safety and a memory leak.

(Actually I was wondering if I could post my whole code sample (about 300 lines) and ask people to critique it for idiomatic C++ usage in general, but I'm not sure whether that kind of question is appropriate on stackoverflow.)

---

It is not idiomatic to write your own container: it is rather exceptional, and for the most part useful only for learning how to write containers. At any rate, it is most certainly not idiomatic to use std::autp_ptr with standard containers. In fact, it's wrong, because copies of std::auto_ptr aren't equivalent: only one auto_ptr owns a pointee at any given time.

As for idiomatic use of std::auto_ptr, you should always name your auto_ptr on construction:

int wtv() { /* ... */ }
void trp(std::auto_ptr<int> p, int i) { /* ... */ }

void safe() {
    std::auto_ptr<int> p(new int(12));
    trp(p, wtv());
}

void danger() {
    trp(std::auto_ptr<int>(new int(12)), wtv());
}

Because the C++ standard allows arguments to evaluate in any arbitrary order, the call to danger() is unsafe. In the call to trp() in danger(), the compiler may allocate the integer, then create the auto_ptr, and finally call wtv(). Or, the compiler may allocate a new integer, call wtv(), and finally create the auto_ptr. If wtv() throws an exception then danger() may or may not leak.

In the case of safe(), however, because the auto_ptr is constructed a-priori, RAII guarantees it will clean up properly whether or not wtv() throws an exception.

---

Yes it is.

For example, see the interface of the Boost.Pointer Container library. The various pointer containers all feature an insert function taking an auto_ptr which semantically guarantees that they take ownership (they also have the raw pointer version but hey :p).

There are however other ways to achieve what you're doing with regards to exception safety because it's only internal here. To understand it, you need to understand what could go wrong (ie throw) and then reorder your instructions so that the operations that may throw are done with before the side-effects occur.

For example, taking from your post:

auto_ptr<Node> new_node(new Node(to_insert)); // 1
n->m_children.push_back(new_node.get());      // 2
new_node.release();                           // 3

Let's check each line,

1. The constructor may throw (for example if the CopyConstructor of the type throws), in this case however you are guaranteed that new will perform the cleanup for you
2. The call to push_back may throw a std::bad_alloc if the memory is exhausted. That's the only error possible as copying a pointer is a no-throw operation
3. This is guaranteed not to throw

If you look closely, you'll remark that you would not have to worry if you could somehow have 2 being executed before 1. It is in fact possible:

n->m_children.reserve(n->m_children.size() + 1);
n->m_children.push_back(new Node(to_insert));

The call to reserve may throw (bad_alloc), but if it completes normally you are then guaranteed that no reallocation will occur until size becomes equal to capacity and you try another insertion.

The call to new may fall if the constructor throw, in which case new will perform the cleanup, if it completes you're left with a pointer that is immediately inserted in the vector... which is guaranteed not to throw because of the line above.

Thus, the use of auto_ptr may be replaced here. It was nonetheless a good idea, though as has been noted you should refrain from executing RAII initialization within a function evaluation.

---

I like the idea of declaring the ownership of pointer. This is one of the great features in c++0x, std::unique_ptrs. However std::auto_ptr is so hard to understand and lethal is even slightly misued I would suggest avoiding it entirely.

---

http://www.gotw.ca/publications/using_auto_ptr_effectively.htm