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?

Pecan

Why not: void ClassA::ExportAnimation(const auto_ptr<CAnimation>& animation); No transfer of ownership. – Flexile

The real question is: why don't you like this ? – Jenine

In the first example (which I don't like) the calling method can not longer use the animation object - it has been destroyed. But one cannot see that without looking at the header file. I would expect the object to be deleted when the auto_ptr goes out of scope. If the method takes ownership of the object, it should have a name that describes this behaviour: ExportAndDestroyAnimation – Pecan

edit the question - '...do some more stuff' in classB::someMethod() – Pecan

It doesn't destroy the animation, it assumes ownership of it. If it doesn't need to assume ownership, then you're right, it should not accept an auto_ptr by value. But it also should not accept a raw pointer. – Inhaler

The first method is definitely better. The second method you are exposing a RAW pointer, which is just BAD practice in C++. The whole point of auto_ptr is that when you pass it to a function you are passing ownership either the function has taken ownership and stored it or it has been destroyed. That is exactly what passing an auto_ptr means. If you wanted to use it externally I would consider what ExpportAnimation is doing. Why odes it want ownership. If you fiddle with it externally is that going to mess up something else? Does it provide access to the object or can it be shared? – Buffoon

Answer 1

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.

Circulation

Great answer - I think void ExportAnimation( CAnimation const & ) is the best option for my example - because the ExportAnimation method does not alter the passed object. I think I failed to make this clear in my original question. And I agree a reference is better than a raw pointer. – Pecan

Actually, I don't see any issue with a raw pointer: I just treat it as a nullable reference and certainly doesn't expect to be passed ownership using it... but it may a local shop convention. I would NOT however implement the release solution as this would involve passing a raw pointer to a function that now need to assume ownership without having a signature saying so. – Juno

Answer 2

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

Fullback

yes - the called method has an new auto_ptr on it's stack which owns the object. There is no bug in the code. But from the readability of the calling code, can I really see that object no longer exists after the method call? I would need to look at the header file to see that the parameter type is an auto_ptr and not a normal pointer. I would expect the object to exist until the auto_ptr goes out of scope. – Pecan

Depends on how you define "unambiguously", doesn't it? Could be that someone just thought that was the right thing to do with an auto_ptr, treating it like a normal pointer. Since the object would have fallen out of scope anyway on the very next line, and we don't see the ExportAnimation function, we can't say whether that's what the other guys intended or they just didn't know how to use auto_ptr's. – Bioluminescence

@Bioluminescence When not assuming correct code (or comments), there is not much (nothing?) left that holds anyway. – Fullback

@GarethOwen: Completely disagree. You do not need to look at the header file. You have an auto_ptr in your function (so you know it is an auto_ptr). So when you call the method you also know that you are passing an auto_ptr. This means that ownership has passed to the controlling method. Even if the method took a reference to an auto_ptr then you should be aware that the auto_ptr may contain NULL on return. The whole point is that auto_ptr means if you pass it to somebody else you are giving up ownership. – Buffoon

@Martin: You're right - I wrongly thought that if I pass an auto_ptr to a method that expects a normal pointer, then the pointer is passed, even if I do not write .get(). But in actual fact I need to write .get() or else I get a compile error, so I can see when an auto_ptr is being passed. (At least with VS2008 compiler) – Pecan

Answer 3

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
}

Eleonoreleonora

the point of the auto_ptr here could be exception safety - the object will still be correctly destructed in the event of an exception in ExportAnimation. – Pecan

@Gareth: Fair 'nuff. But if the object is guaranteed to exist upon the function's return (which exception safety requires in the case of passing a raw pointer), it belongs on the stack anyway. – Eleonoreleonora

+1 @Eleonoreleonora - the code in you're edited answer is how I would write it also. – Pecan

Answer 4

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.

Elaelaborate