Code Review question - should I allow this passing of an auto_ptr as parameter?

Question

Consider the following example code which I have recently seen in our code base:

void ClassA::ExportAnimation(auto_ptr<CAnimation> animation)
{
... does something
}

// calling method:
void classB::someMethod()
{
  auto_ptr<CAnimation> animation (new CAnimation(1,2));
  ClassA classAInstance;
  classAInstance.ExportAnimation(animation)
  ... do some more stuff
}

I don't like this - and would rather write it so:

void ClassA::ExportAnimation(CAnimation* animation)
{
    ... does something
}

// calling method:
void classB::someMethod()
{
  auto_ptr<CAnimation> animation (new CAnimation(1,2));
  ClassA classAInstance;
  classAInstance.ExportAnimation(animation.get())
  ... do some more stuff
}

but it is really a problem?

Answer 1

The auto_ptr unambiguously declares that the ownership of the pointer is passed on. The plain pointer isn't self-documenting.

Answer 2

What is the point of an auto-ptr if you only use its internals as a storage location?

Yes, pass it to the function. Or do away with it entirely, if you really don't want it. Presumably the function needs it to pass along ownership to something else.

It sounds like maybe the alternative you're looking for is much simpler:

void ClassA::ExportAnimation(CAnimation &animation) // no pointer

// calling method:
void classB::someMethod()
{
  CAnimation animation(1,2); // no pointer
  ClassA classAInstance;
  classAInstance.ExportAnimation(animation) // no ownership tranfer
  ... do some more stuff
  // object dies here, no earlier, no later
}

Answer 3

Passing the smart pointer to ExportAnimation clearly documents, and enforces, that ownership has been passed to the function, and there is no need for the caller to delete the animation. The function will also not need to explicitly delete the object, just let the pointer go out of scope.

Your suggestion leaves that ambigious; should ExportAnimation delete the object you've passed via raw pointer? You'd need to check the function's documentation to know what the caller should do, and also check the implementation to make sure it's actually implemented as documented.

I would always recommend using smart pointers (and other RAII idioms) to make object lifetime explicit and automatic.

Answer 4

It all depends on what ExportAnimation is and how it is implemented.

Does it only use the object for the duration of the call and then leaves it?

Then convert to a reference and pass a real reference. There is no need to pass membership and the argument is not optional, so void ExportAnimation( CAnimation const & ) suffices. The advantage is that it is clear from the interface that there is no memory management issues with the method, it will just use the passed object and leave it as such. In this case, passing a raw pointer (as in your proposed code) is much worse than passing a reference in that it is not clear whether ExportAnimation is or not responsible for deletion of the passed in object.

Does it keep the object for later use?

This could be the case if the function starts a thread to export the animation in the background. In this case, it has to be clear that the lifetime of the argument must extend beyond the duration of the call. This can be solved by using shared_ptr --both in the function and outside of it-- as they convey the object is shared and will be kept alive as much as required meaning. Or else you can actually transfer ownership.

In the later case, if transfer of ownership is performed, then the initial code is fine --the signature is explicit in the ownership transfer. Else you can opt to document the behavior, change to a raw pointer and make the transfer explicit by calling ExportAnimation( myAnimation.release() ).

You have added some concerns as a comment to another answer:

can I really see that object no longer exists after the method call?

The caller auto_ptr is reset to 0 in the call, so any dereference will kill be an error and will be flagged in the first test you try.

I would need to look at the header file to see that the parameter type is an auto_ptr and not a normal pointer.

You do not need to look at the header... just try passing a raw pointer and the compiler will tell you that it requires an auto_ptr<> --There is no implicit conversion from raw pointer to auto_ptr.

I would expect the object to exist until the auto_ptr goes out of scope.

The standard auto_ptr, unlike boost::scope_ptr, do not have that semantics. The ownership of the object can be released or passed to other auto_ptr, so the assumption that an object held in an auto_ptr lives for the whole scope of the auto_ptr is bad in itself.