Sign in to follow this  

std::unique_ptr issues

This topic is 1189 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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?

Edited by Juliean

Share this post


Link to post
Share on other sites

I don't understand the problem you describe. Could you post a minimal complete program that shows the C2027 error?

 

Of course. Minimal program to show this would be:

#include <unordered_map>
#include <memory>

struct Bar;

struct Foo
{
	std::unordered_map<std::wstring, std::unique_ptr<Bar>> m_mPointer;
};

int main(){

	Foo foo;

	return -1;
}

Doesn't matter whether I put Foo.h in seperate file, and whether "Bar" exists in the translation unit and/or is included to Foo.cpp or not. The error is given by a static assert in the unique_ptr's default destructor:

	void operator()(_Ty *_Ptr) const _NOEXCEPT
		{	// delete a pointer
		static_assert(0 < sizeof (_Ty),
			"can't delete an incomplete type");
		delete _Ptr;
		}

Its basically just a safety-check for the warning that would be otherwise issues (C4150) and the resulting memory leak/undefined behaviour. I've actually written my own unique_ptr implementation, which doesn't do the static_assertion to confirm this.

 

Now I know that you technically CANT delete a class whose definition you don't have. I'm asking this since I've read that this can work if you make an empty dtor for the class and define it in the cpp-file that includes the forward declared class. This doesn't seem to work for me, is there still a way to use the unique_ptr without having to include the full class definiton to the header?

 

Is it understandable now?

Edited by Juliean

Share this post


Link to post
Share on other sites
You can't delete pointers to Bar if you don't have the definition of Bar. There is nothing special about that in std::unique_ptr. This compiles just fine:
 
#include <unordered_map>
#include <memory>
#include <string>

struct Bar;

struct Foo {
  std::unordered_map<std::wstring, std::unique_ptr<Bar>> m_mPointer;
};

struct Bar {
};

int main(){
  Foo foo;
}

Share this post


Link to post
Share on other sites

You can't delete pointers to Bar if you don't have the definition of Bar. There is nothing special about that in std::unique_ptr. This compiles just fine:

 

Hm, I kind of was afraid that was the case. This is unfortunate, because otherwise it would have been like this:

// Foo.h

struct Bar;

struct Foo
{
    ~Foo(void);

    Bar* m_pBar;
}

// Foo.cpp
#include "Bar.h"

Foo::~Foo(void)
{
    delete m_bar;
}

While now I have to do

// Foo.h

#include "Bar.h"
#include <memory>

struct Foo
{
     std::unique_ptr<Bar> m_bar;
};

Which really makes me rethink the decision of applying those pointer everwhere. I liked the idea of automatic memory management without overhead, but not at the cost of losing forward declaration wherever I need it... :/

Edited by Juliean

Share this post


Link to post
Share on other sites


This is unfortunate, because otherwise it would have been like this:

// Foo.h

struct Bar;

struct Foo
{
~Foo(void);

Bar* m_pBar;
}

// Foo.cpp
#include "Bar.h"

Foo::~Foo(void)
{
delete m_bar;
}

Not sure why you are saying that. Which compiler are you using?

 

This works with VS2013 update 3

//////Test.h

#include <memory>

struct Bar;
struct Foo
{
	~Foo(void);
	std::unique_ptr<Bar> m_pBar;
};

/////Test.cpp
#include "test.h"

struct Bar
{

};

int main()
{
	Foo f;
}

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

Share this post


Link to post
Share on other sites
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).

The examples provided above by Alvaro, Juliean and N.I.B. work because, by the time a Foo is instantiated (in main) the definition of Bar is available, whereas in your example Bar is never defined.

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. Edited by SmkViper

Share this post


Link to post
Share on other sites
There's a proposed paper to add support for containers of smart pointers to incomplete types. No standard library implements it yet.

In other cases, remember that the compiler generates some implicit functions that invoke member destructors, like the destructor and copy operators. Explicitly declaring these functions in the declaration and then defining the functions as =default in the source tile avoids some problems, but had trade-offs.

edit: fixed some mangling my phone did when originally writing this up. Edited by SeanMiddleditch

Share this post


Link to post
Share on other sites


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...

Share this post


Link to post
Share on other sites
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. Edited by Aardvajk

Share this post


Link to post
Share on other sites

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;
}
Edited by Pink Horror

Share this post


Link to post
Share on other sites

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.

Edited by Juliean

Share this post


Link to post
Share on other sites

This topic is 1189 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this