Back to General and Gameplay Programming

std::unique_ptr issues

Julian Watzinger · 2014-09-12T02:54:22

Hello, I'm just in the process of replacing objects that owned and deleted a pure pointer with std::unique_ptr. However, I'm running into some issues: - I've been heavily using forward declaration to speed up compile times, like here: class Trigger; class Instance { typedef std::unordered_map<std::wstring, Trigger*> TriggerMap; public: ~Instance(void) { for(auto& trigger : m_mTrigger) { delete trigger.second; } } private: TriggerMap m_mTrigger; } However, I keep getting "warning C2027" in visual studio, in that an incomplete type can't be deleted when replacing the pure pointer with std::unique_ptr. Some of the times leaving the empty dtor fixes it, however in most cases, especially containers, I have to replace the forward-declaration with a direct include in the header. Is there some way around this? It really bugs me having to do this on so many occasions, could make recompile times significantly larger (already kind of a problem since I'm heavily using templates)... class Trigger; class Instance { typedef std::unordered_map<std::wstring, std::unique_ptr<Trigger>> TriggerMap; public: ~Instance(void); // defined in cpp-file private: TriggerMap m_mTrigger; } - I also keep getting compilation-errors with this kind of container-of-pointer setup when the class has a default copy ctor/operator. So the above example wouldn't even compile unless I put: class Trigger; class Instance { typedef std::unordered_map<std::wstring, std::unique_ptr<Trigger>> TriggerMap; public: Instance(void) = default; Instance(const Instance&) = delete; ~Instance(void); // defined in cpp-file void operator=(const Instance&) = delete; private: TriggerMap m_mTrigger; } Now I don't completely mind it, in fact I have been very sloppy about doing this before, since a class the won't compile that way shouldn't have been copyable before. Still, I wonder whether there is any shorthand to this? E.g. adding a std::unique_ptr member will automatically disable copy ctor/operator for the class, I wonder if thats possibly within a container class too?

General and Gameplay Programming Programming

Started by Juliean September 08, 2014 02:06 PM

13 comments, last by mpratscher 9 years, 7 months ago

Juliean

7,343

Author

September 09, 2014 08:09 AM

I actually use unique_ptr with forward declarations a lot. Works great.

Ah, I see, so the definition has to be available at the point where I instanciate the class then. Thats only half-bad. Also explains why half my classes "mysteriously" worked with forward-declaration and the others didn't.

Your problem seems to be something causing std::unique_ptr to instantiate the destructor before Bar is defined. Pay close attention to the warning to see if it indicates another file in your compilation unit. This will usually happen if you have code in your header file that is trying to manipulate the unique_ptr, even if the manipulation is "invisible" (i.e. constructor, destructor, accessing, etc).

Unfortunately the warning at the end of the template-error only pointed to the member variable declaration itself, but maybe I missed something :/

Also, unique_ptr is not copyable, so anything that stores them can not be copied. This includes containers, which is why compilation failed with the auto-generated copy constructor and copy assignment operator. Since unique_ptr indicates single-ownership, this is desired, and you should delete the copy constructor and assignment operator as you did above. If you want to allow someone to move ownership, then supply your own move constructor and operator which should work just fine.

Yeah, I get that, but on the same time it appears inconsistent that having a reference or unique_ptr as member will implicitely disallow copying without any warning/errors (unless you try to copy, duh) but having it in a container will generate compile errors, even if I never try to compile the class. I also kind of disliked it because at first I thought using unique_ptr would remove a lot of unnecessary code... which it does in the cpp-files with all the deletes gone, and now that I know the cause for the undefined-type-warning I can get rid of all the empty dtors, but still I have to bloat the interface a little with all the default/delete-methods. But I quess you could argue that it was a fault for me not doing this before anyways, since being able to copy a class with dynamic memory with default copy-ctor sucks by itself.

I suspect all your problems would disappear if you used a pimpl. Hide your private parts.

That would indeed solve the issue, and speed up compile time by another factor, but I'm not quite willing to do that. I've already considered using pimpl, but I decided against in mostly due to performance concerns and additional development overhead, that currently stands in no relation to the gain. Regarding performance, I know its probably "premature optimization" in a way, but with all the talks about cache coherency & memory fregmentation, I'm certain that for most parts I'm better of without it. I just recently squished out about 0,1 ms by constructing my state machine statically instead of via new, so yeah...

Aardvajk

13,217

September 09, 2014 11:34 AM

I came across something similar recently and found adding a blank destructor to the owning class, with its body defined in the .cpp after the inclusion of the full header meant I could just forward define the type in the header and declare one of these templated pointers. As soon as I remove the destructor from the .cpp and let the compiler auto-generate it or have it stubbed as an inline empty destructor, the warning comes back.

So yes, it is to do with when the compiler generates the destruction code. The full class needs to be visible at that point, but (on GCC at least) if you have a (possibly empty) destructor in the .cpp that seems to fix it.

Not sure if this is fully defined behaviour or not though.

Really its like this:


class B;

class A
{
public:
    ~A(){ delete b; }

    B *b;
};

Clearly not going to work as b is undefined at the point of the delete.


class B;

class A
{
public:
    ~A();

    B *b;
};

// cpp
#include "B.h"

A::~A(){ delete b; }

Is clearly fine. The fact you are using template smart pointers is kind of irrelevant really.

journal | website | twitter | youtube

Pink Horror

2,459

September 10, 2014 04:19 AM

Yeah, I get that, but on the same time it appears inconsistent that having a reference or unique_ptr as member will implicitely disallow copying without any warning/errors (unless you try to copy, duh) but having it in a container will generate compile errors, even if I never try to compile the class. I also kind of disliked it because at first I thought using unique_ptr would remove a lot of unnecessary code... which it does in the cpp-files with all the deletes gone, and now that I know the cause for the undefined-type-warning I can get rid of all the empty dtors, but still I have to bloat the interface a little with all the default/delete-methods. But I quess you could argue that it was a fault for me not doing this before anyways, since being able to copy a class with dynamic memory with default copy-ctor sucks by itself.

The default methods are also a problem.

Any code that constructs a unique_ptr<T> has to also construct the default deleter of T, which has that static_assert with the sizeof(T), which means it needs the complete type of T. Putting a default constructor in your container class causes the problem just as much as the default destructor. Your constructor, destructor, and really anything else that requires any detail of the class T should go into the .cpp file that implements your container. By putting the constructor and destructor into the .cpp file, you should be able to save compile time (they'll only be compiled in one place) and solve this problem.

This compiles (if you don't call the code, it doesn't need to exist):


class Member;
struct Test
{
    //Test();
    //~Test();
    std::unique_ptr<Member> mPtr;
};
int main()
{
    //Test t;
    return 0;
}

This is just a linker error (you can fix the problem with another .cpp file):


class Member;
struct Test
{
    Test();
    ~Test();
    std::unique_ptr<Member> mPtr;
};
int main()
{
    Test t;
    return 0;
}

And this is a compile error (compiling the default constructor needs to actually try to implement parts of the unique_ptr with an incomplete type, which opens a whole can of worms):


class Member;
struct Test
{
    //Test();
    ~Test();
    std::unique_ptr<Member> mPtr;
};
int main()
{
    Test t;
    return 0;
}

Juliean

7,343

Author

September 11, 2014 09:29 AM

Alright, having the empty dtor AND and empty default ctor does the trick. That still makes things a little more ugly than I hoped too but well, its better than removing forward-declarations. Its not really a disadvantage, I would have had to define a ctor/dtor when using a raw pointer too, I was just hoping to get rid of them completely. Nevermind, I can work that way now, thanks to all.