Is this a fine std::auto_ptr<> use case?

Question

Hello all,

Please suppose I have a function that accepts a pointer as a parameter. This function can throw an exception, as it uses std::vector<>::push_back() to manage the lifecycle of this pointer. If I declare it like this:

void manage(T *ptr);

and call it like this:

manage(new T());

if it throws an exception pushing the pointer into the std::vector<>, I effectively have got a memory leak, haven't I?

Would declaring the function like this:

void manage(std::auto_ptr<T> ptr);

solve my problem?

I would expect it to first allocate the std::auto_ptr on the stack (something that I guess couldn't ever throw an exception) and let it acquire ownership over the pointer. Safe.

Then, inside the function, I would push the raw pointer into the std::vector<>, which is also safe: if this fails, the pointer will not be added, but the smart pointer will still be owning the pointer so it will be destroyed. If the pushing succeeds, I would remove the smart pointer's ownership over that pointer and return: this can't throw an exception, so it will always be fine.

Are my theories correct?

-- Edit --

No, I think I can't do that. Doing that would require taking a non-const reference to rvalue (to take away ownership from the smart pointer). I would have to write

std::auto_ptr<T> ptr(new T());
manage(ptr);

for that to work, something that's quite inconvient in my case. I'm writing this so that I can implement RAII without polluting much the code. Doing that thing would not help, then. That would be catch 22.

-- Edit 2 --

Pulling what Jason Orendorff said down there here for quick reference by readers, the ultimate solution seems to be the following:

void manage(T *ptr)
{
    std::auto_ptr<T> autoPtr(ptr);
    vector.push_back(ptr);
    autoPtr.release();
}

This solves the problem of the useless non-const reference to rvalue.

When I finish this class I'm coding I'll post it back here in case anyone finds it useful.

-- Edit 3 --

Ok, there has been a lot of discussion here, and there are key points that I should have clarified before. In general, when I post at stackoverflow, I try to explain the reason behind my questions and, in general, that is completely useless. So this time I thought I should go straight to the point. Turns out it didn't work quite well XD

Unfortunately my brain is deadlocking now, so I think I won't even be able to explain correctly what I first thought to accomplish my goals. I am trying to find a good solution for atomic operations and exception-safe code writing that fits lots of cases, but really, I can't handle it XD I think this is the kind of thing I will only master with time.

I am a really new C++ programmer and my focus is game development. When an exception is thrown in a game engine, it is the end of execution. The system will free all the memory for my process, so it doesn't really matter if one or two pointers leak here and there. Now that I am developing a server application, I'm finding it hard to deal with exceptions, because an exception can't crash the server; it must "crash the request".

That is, "Well, client, unfortunately the developers didn't foresee this condition, so you'll have to try it later (up to here, it is basically the same thing as with a game engine, nothing is being repared, only it is isolated to the context of just a request, and not the whole process). But don't panic, as everything was left in a valid state (here is one of the differences, however. The process isn't terminated, so the operating system can't free the resources for you; furthermore, you have to pay attention to undo the operations so far, so that you don't lock completely an user's account, for example, or even a full service provided by the server).".

I will just code more and more and note down my problems so that I can write a better question next time. I wasn't prepared to ask this now, I'm really sorry.

Thanks a lot for your replies, I really like stackoverflow. It is absolutely amazing how fast my questions are answered and how enlightening your answers are. Thanks.

Answer 1

You could do it this way, but you still have to cleanup in the event of an exception not being thrown, which seems a little onerous.

If you use something like boost::shared_ptr (I believe something like this is in TR1 libraries as well - as an example see MS's implementation) you could forget about having to cleanup in the event of things going as planned.

To make this work you would need to make your vector accept boost::shared_ptr < T > instance, then you would just let your original instance be cleaned up and it would leave instance in the vector alive in the case that all goes well. In the case things go wrong all boost::shared_ptr instances would be destroyed and you would still end up with no leak.

With smart pointers it's about picking one that suits the task, and in this case shared ownership (or simple ownership transfer) seems to be a goal so there are better candidates than std::auto_ptr on most platforms.

Answer 2

Yes, that's fine. (See edit below.) (jkp might be seeing something I've missed, but I don't think you "still have to clean up in the event of an exception being thrown" because, as you say, in that case the auto_ptr will delete the object for you.)

But I think it would be better still to hide the auto_ptr shenanigans from the caller:

void manage(T *t) {
    std::auto_ptr<T> p(t);  // to delete t in case push_back throws
    vec.push_back(t);
    p.release();
}

EDIT: I initially wrote "Yes, that's fine," referring to the original manage(auto_ptr<T>) plan, but I tried it and found that it doesn't work. The constructor auto_ptr<T>::auto_ptr(T *) is explicit. The compiler wouldn't let you write manage(new T) because it can't implicitly convert that pointer to an auto_ptr. manage(T *) is a friendlier interface anyway!

Answer 3

In general, using std::auto_ptr as a type of function argument unambiguously tells the caller that function will take ownership of the object, and will be responsible for deleting it. If your function fits that description, then by all means use auto_ptr there regardless of any other reason.

Answer 4

I think enough discussion has been generated to warrant yet another answer.

Firstly, to answer the actual question, yes, it is absolutely appropriate (and even necessary!) to pass an argument by smart pointer when ownership transfer occurs. Passing by a smart pointer is a common idiom to accomplish that.

void manage(std::auto_ptr<T> t) {
    ...
}
...

// The reader of this code clearly sees ownership transfer.
std::auto_ptr<T> t(new T);
manage(t);

Now, there is a very good reason why all smart pointers have explicit constructors. Consider the following function (mentally replace std::auto_ptr with boost::shared_ptr if it tickles your fancy):

void func(std::auto_ptr<Widget> w, const Gizmo& g) {
    ...
}

If std::auto_ptr had an implicit constructor, suddenly this code would compile:

func(new Widget(), gizmo);

What's wrong with it? Taking almost directly from "Effective C++", Item 17:

func(new Widget(), a_function_that_throws());

Because in C++ order of argument evaluation is undefined, you can very well have arguments evaluated in the following order: new Widget(), a_function_that_throws(), std::auto_ptr constructor. If a function throws, you have a leak.

Therefore all resources that will be released need to be wrapped upon construction in RAII classes before being passed into a function. This means all smart pointer have to be constructed before passing them as an argument to a function. Making smart pointers be copy-constructible with a const reference or implicitly-constructible would encourage unsafe code. Explicit construction enforces safer code.

Now, why shouldn't you do something like this?

void manage(T *ptr)
{
    std::auto_ptr<T> autoPtr(ptr);
    vector.push_back(ptr);
    autoPtr.release();
}

As already mentioned, the interface idioms tell me that I can pass a pointer that I own and I get to delete it. So, nothing stops me from doing this:

T item;
manage(&t);

// or
manage(&t_class_member);

Which is disastrous, of course. But you'd say "of course I know what the interface means, I'd never use it that way". However you may want to add an extra argument to a function later. Or someone (not you, or you 3 years later) comes along this code, they may not see it the same way as you do.

1. This hypothetical "someone else" may only see a header without a comment and (rightly) presume the lack of ownership transfer.
2. They may see how this function is used in some other code and replicate the usage without looking at the header.
3. They may use code auto-completion to invoke a function and not read the comment or the function and presume the lack of ownership transfer.
4. They may write a function that wraps you manage function and yet someone else will use the wrapper function and will miss the documentation of the original function.

5. They may try to "extend" you code so that all the old code compiles (and automatically becomes unsafe):

   void manage(T *t, const std::string tag = some_function_that_throws());
   

As you can see, explicit construction of a smart pointer makes it much harder to write unsafe code in the above cases.

Therefore, I would not recommend going against decades of C++ expertise to make perceivably "nicer" and "fun" API.

My 2c.