std::unique_ptr with an incomplete type won't compile
Asked Answered
S

6

272

I'm using the pimpl-idiom with std::unique_ptr:

class window {
  window(const rectangle& rect);

private:
  class window_impl; // defined elsewhere
  std::unique_ptr<window_impl> impl_; // won't compile
};

However, I get a compile error regarding the use of an incomplete type, on line 304 in <memory>:

Invalid application of 'sizeof' to an incomplete type 'uixx::window::window_impl'

For as far as I know, std::unique_ptr should be able to be used with an incomplete type. Is this a bug in libc++ or am I doing something wrong here?

Symbolic answered 31/3, 2012 at 9:3 Comment(3)
Reference link for completeness requirements: https://mcmap.net/q/64710/-is-std-unique_ptr-lt-t-gt-required-to-know-the-full-definition-of-tDenison
A pimpl is often constructed and not modified since then. I usually use a std::shared_ptr<const window_impl>Endodontics
Related: I would very much like to know why this works in MSVC, and how to prevent it from working (so that I do not break my GCC colleagues' compilations).Finitude
I
355

Here are some examples of std::unique_ptr with incomplete types. The problem lies in destruction.

If you use pimpl with unique_ptr, you need to declare a destructor:

class foo
{ 
    class impl;
    std::unique_ptr<impl> impl_;

public:
    foo(); // You may need a def. constructor to be defined elsewhere

    ~foo(); // Implement (with {}, or with = default;) where impl is complete
};

because otherwise the compiler generates a default one, and it needs a complete declaration of foo::impl for this.

If you have template constructors, then you're screwed, even if you don't construct the impl_ member:

template <typename T>
foo::foo(T bar) 
{
    // Here the compiler needs to know how to
    // destroy impl_ in case an exception is
    // thrown !
}

At namespace scope, using unique_ptr will not work either:

class impl;
std::unique_ptr<impl> impl_;

since the compiler must know here how to destroy this static duration object. A workaround is:

class impl;
struct ptr_impl : std::unique_ptr<impl>
{
    ~ptr_impl(); // Implement (empty body) elsewhere
} impl_;
Islamism answered 31/3, 2012 at 9:8 Comment(13)
I find your first solution (adding the foo destructor) allows the class declaration itself to compile, but declaring an object of that type anywhere results in the original error ("invalid application of 'sizeof'...").Wasserman
@user192737: Can you please post a complete example somewhere reproducing the error (pastebin or ideone or whatever) ?Islamism
Sure! It's just your first example, with a main() in which the class is instantiated: pastebin.com/65jMYzsi I have subsequently found that adding a default constructor to foo makes the error go away - I'm not sure why.Wasserman
@user192737: When the default constructor is generated, impl is not complete, and trying to construct a unique_ptr with an incomplete type is what gave you the error too. Thanks for this, I'll update the answer.Islamism
excellent answer, just to note; we can still use the default constructor/destructor by placing e.g. foo::~foo() = default; in the src fileCaesarism
One way to live with template constructors would be to declare but not define the constructor in the class body, define it somewhere the complete impl definition is seen and explicitly instantiate all the necessary instantiations there.Pericarp
Could you explain how this would work in some cases and would not in others? I have used the pimpl idiom with a unique_ptr and a class with no destructor, and in another project my code fails to compile with the error OP mentioned..Farce
It seems if the default value for unique_ptr are set to {nullptr} in the header file of the class with c++11 style, a complete declaration is also needed for the above reason.Platypus
I had the error even with the destructor declared in cpp, but I realized I had a impl_.reset() inlined in my class header. I moved that call to the cpp and it fixed the problem.Marylynnmarylynne
I have always dealt with this by defining the destructor in the implementation file (rather than the header), for the reasons described in this answer. But I just hit a case where I also had to define a default constructor there as well. I'm not sure what makes this case different than all the other times.Tritheism
Here is an explanation that why "default constructor" of the out-most class (e.g. foo here) is also required to be moved to a place that impl is defined. developercommunity.visualstudio.com/t/…Belew
Important to note: When you use the destructor solution, you may not have any initialization of the impl (like defining it with {})!Syncretize
I just encountered this problem even with the destructor explicitly implemented in the .cpp. It turns out that you need to do this for the move assignment operator as well in at least some cases. (It appeared to not be needed when a consuming class also used compiler-generated move assignment, but when this was changed to hand-rolled it became necessary.)Desdamona
N
72

As Alexandre C. mentioned, the problem comes down to window's destructor being implicitly defined in places where the type of window_impl is still incomplete. In addition to his solutions, another workaround that I've used is to declare a Deleter functor in the header:

foo.h:

class Foo
{
...
private:
  class FooImpl;
  struct FooImplDeleter
  {
    void operator()(FooImpl *p);
  };

  std::unique_ptr<FooImpl, FooImplDeleter> impl_;
};

foo.cpp:

class Foo::FooImpl
{
  // ...
};

void Foo::FooImplDeleter::operator()(Foo::FooImpl *p)
{
  delete p;
}

Note that using a custom Deleter function precludes the use of std::make_unique (available from C++14), as already discussed here.

Nahuatl answered 30/7, 2013 at 14:56 Comment(5)
This is the correct solution as far as I'm concerned. It isn't unique to using the pimpl-idiom, it's a general problem with using std::unique_ptr with incomplete classes. The default deleter used by std::unique_ptr<X> attempts to do "delete X", which it can't do if X is a forward declaration. By specifying a deleter function, you can put that function in a source file where the class X is completely defined. Other source files can then use std::unique_ptr<X, DeleterFunc> even though X is just a forward declaration as long as they are linked with the source file containing DeleterFunc.Interoffice
This is a good workaround when you must have an inline function definition creating an instance of your "Foo" type (for instance a static "getInstance" method that references constructor and destructor), and you do not want to move these into an implementation file as @adspx5 suggests.Selectivity
The deleter class may be the only proper solution in some cases. I personally use an extended make_unique using a deleter class: template<typename _Tp, typename _Deleter, typename... _Args> auto make_unique_with_deleter(_Args&&... __args) { return std::unique_ptr<_Tp, _Deleter>(new _Tp(std::forward<_Args>(__args)...), _Deleter{}); }Savanna
Ten years late, I know – but I'd prefer making FooImplDeleter a nested class of Foo – it's just an implementation detail and shouldn't be visible outside, just as actually the FooImpl class shouldn't be either.Pederasty
I totally agree @Aconcagua, thanks for the suggestion. I changed the impl classes to be nested inside Foo.Nahuatl
J
34

use a custom deleter

The problem is that unique_ptr<T> must call the destructor T::~T() in its own destructor, its move assignment operator, and unique_ptr::reset() member function (only). However, these must be called (implicitly or explicitly) in several PIMPL situations (already in the outer class's destructor and move assignment operator).

As already pointed out in another answer, one way to avoid that is to move all operations that require unique_ptr::~unique_ptr(), unique_ptr::operator=(unique_ptr&&), and unique_ptr::reset() into the source file where the pimpl helper class is actually defined.

However, this is rather inconvenient and defies the very point of the pimpl idoim to some degree. A much cleaner solution that avoids all that is to use a custom deleter and only move its definition into the source file where the pimple helper class lives. Here is a simple example:

// file.h
class foo
{
    struct pimpl;
    struct pimpl_deleter { void operator()(pimpl*) const; };
    std::unique_ptr<pimpl,pimpl_deleter> m_pimpl;
  public:
    foo(some data);
    foo(foo&&) = default;             // no need to define this in file.cc
    foo&operator=(foo&&) = default;   // no need to define this in file.cc
  //foo::~foo()          auto-generated: no need to define this in file.cc
};

// file.cc
struct foo::pimpl
{
  // lots of complicated code
};
void foo::pimpl_deleter::operator()(foo::pimpl*ptr) const { delete ptr; }

Instead of a separate deleter class, you can also use a free function or static member of foo:

class foo {
    struct pimpl;
    static void delete_pimpl(pimpl*);
    using deleter = void(&)(pimpl*);
    std::unique_ptr<pimpl,deleter> m_pimpl{nullptr,delete_pimpl};
  public:
    foo(some data);
};

Although this requires m_pimpl to hold the address of the deleter, requiring extra storage that is not needed in the first example.

Joby answered 28/8, 2015 at 10:52 Comment(3)
I like your very last example. I would be glad it to work as you wrote it. But the declaration of std::unique_ptr expects type of the deleter as a second template argument and not the deleter object itself. At least my MSVC v16 complains.Tasha
@Ivan_Bereziuk Yes, that code was wrong. Fixed it now. Thanks for pointing that out.Joby
Note that the line std::unique_ptr<pimpl,deleter> m_pimpl in the second example should actually be changed to: std::unique_ptr<pimpl,deleter> m_pimpl { nullptr, delete_pimpl }. Otherwise, the default initialization of m_pimpl will fail. Furthermore, in the second example, the storage size of the unique_ptr is doubled, because it has to hold a pointer to the deleter function right from the construction. This doesn't happen in the first example, where the deleter object is default constructible and takes zero size, when it is merged into the unique_ptr.Sanatory
D
24

Probably you have some function bodies within .h file within class that uses incomplete type.

Make sure that within your .h for class window you have only function declaration. All function bodies for window must be in .cpp file. And for window_impl as well...

Btw, you have to explicitly add destructor declaration for windows class in your .h file.

But you CANNOT put empty dtor body in you header file:

class window {
    virtual ~window() {};
  }

Must be just a declaration:

  class window {
    virtual ~window();
  }
Durbar answered 12/4, 2014 at 15:56 Comment(1)
This was my solution as well. Way more concise. Just have your constructor/destructor declared in header and defined in the cpp file.Outspan
K
5

To add to the other's replies about the custom deleter, in our internal "utilities library" I added a helper header to implement this common pattern (std::unique_ptr of an incomplete type, known only to some of the TU to e.g. avoid long compile times or to provide just an opaque handle to clients).

It provides the common scaffolding for this pattern: a custom deleter class that invokes an externally-defined deleter function, a type alias for a unique_ptr with this deleter class, and a macro to declare the deleter function in a TU that has a complete definition of the type. I think that this has some general usefulness, so here it is:

#ifndef CZU_UNIQUE_OPAQUE_HPP
#define CZU_UNIQUE_OPAQUE_HPP
#include <memory>

/**
    Helper to define a `std::unique_ptr` that works just with a forward
    declaration

    The "regular" `std::unique_ptr<T>` requires the full definition of `T` to be
    available, as it has to emit calls to `delete` in every TU that may use it.

    A workaround to this problem is to have a `std::unique_ptr` with a custom
    deleter, which is defined in a TU that knows the full definition of `T`.

    This header standardizes and generalizes this trick. The usage is quite
    simple:

    - everywhere you would have used `std::unique_ptr<T>`, use
      `czu::unique_opaque<T>`; it will work just fine with `T` being a forward
      declaration;
    - in a TU that knows the full definition of `T`, at top level invoke the
      macro `CZU_DEFINE_OPAQUE_DELETER`; it will define the custom deleter used
      by `czu::unique_opaque<T>`
*/

namespace czu {
template<typename T>
struct opaque_deleter {
    void operator()(T *it) {
        void opaque_deleter_hook(T *);
        opaque_deleter_hook(it);
    }
};

template<typename T>
using unique_opaque = std::unique_ptr<T, opaque_deleter<T>>;
}

/// Call at top level in a C++ file to enable type %T to be used in an %unique_opaque<T>
#define CZU_DEFINE_OPAQUE_DELETER(T) namespace czu { void opaque_deleter_hook(T *it) { delete it; } }

#endif
Kulun answered 13/2, 2018 at 14:19 Comment(0)
P
4

May be not a best solution, but sometimes you may use shared_ptr instead. If course it's a bit an overkill, but... as for unique_ptr, I'll perhaps wait 10 years more until C++ standard makers will decide to use lambda as a deleter.

Another side. Per your code it may happen, that on destruction stage window_impl will be incomplete. This could be a reason of undefined behaviour. See this: Why, really, deleting an incomplete type is undefined behaviour?

So, if possible I would define a very base object to all your objects, with virtual destructor. And you're almost good. You just should keep in mind that system will call virtual destructor for your pointer, so you should define it for every ancestor. You should also define base class in inheritance section as a virtual (see this for details).

Piecework answered 30/11, 2018 at 8:1 Comment(0)

© 2022 - 2024 — McMap. All rights reserved.