TAutoPtr lacks copy/move constructor/operators

Hi,

We noticed that TAutoPtr doesn’t declare copy or move constructors or assignment operators, and doesn’t make them private or delete them either. This is pretty dangerous and caused crashes when we were making changes to FPakPlatformFile’s reader map.

Robert,

The TAutoPtr was modeled after std::auto_ptr. Similarly, there’s a TUniquePtr which pretty much has the behavior you’ve described (and was modeled after std::unique_ptr. Generally speaking anywhere where there’s a TAutoPtr it can probably be upgraded safely to a TUniquePtr.

Thanks,
Jon N.

std::auto_ptr is very careful not to allow an autogenerated copy constructor/assignment operator taking const auto_ptr& to be generated, because this would lead to a double free. At the very least UE’s TAutoPtr should hide/delete these operators to prevent the dangerous auto-generated ones.

Ref: std::auto_ptr - cppreference.com

Yes, but what Rob is saying is that TAutoPtr is actually used in containers in your code and that it’s probably not a good idea to do that.

If it’s ‘modeled’ after std:auto_ptr, then it shouldn’t be possible to use it in a container:
Copying an auto_ptr copies the pointer and transfers ownership to the destination: both copy construction and copy assignment of auto_ptr modify their right hand arguments, and the “copy” is not equal to the original. Because of these unusual copy semantics, auto_ptr may not be placed in standard containers. std::unique_ptr is preferred for this and other uses. (since C++11)

Either make it safe to use in containers, or make it impossible to use in containers… but definitely don’t use it in containers in UE4.

Robert and Aurelien,

Right, I can see how this leads to bugs. I pointed it out to our core team and the response they gave was what I mentioned. I’m not sure if this was just an oversight in the initial design, or something that was intentional (albeit dangerous).

My guess is that it was put in initially, used, and then UniquePtr was implemented later as the preferred alternative in our code base. My guess is that the upgrade policy was the same as most “legacy” like things: “upgrade when the offending code is modified”. The only problem being that not everyone might have realized that it was unsafe if they didn’t look at the implementation.

I’ve gone ahead and created a JIRA for this though: UE-34117

Thanks,
Jon N.

Rather than fix TAutoPtr, we will likely remove it in a future engine version. It should definitely be regarded as legacy and TUniquePtr should be used instead.

Steve