62

Here is a simplification of what I'm seeing when I try to use unique_ptr for pimpl. I chose unique_ptr because I really want the class to own the pointer - I want the lifetimes of the pimpl pointer and the class to be the same.

Anyway, here is the header:

#ifndef HELP
#define HELP 1

#include <memory>

class Help
{

public:

  Help(int ii);
  ~Help() = default;

private:

  class Impl;
  std::unique_ptr<Impl> _M_impl;
};

#endif // HELP

Here is the source:

#include "Help.h"

class Help::Impl
{
public:
  Impl(int ii)
  : _M_i{ii}
  { }

private:

  int _M_i;
};

Help::Help(int ii)
: _M_impl{new Help::Impl{ii}}
{ }

I could compile these into a library just fine. But when I try to use it in a test program I get

ed@bad-horse:~/ext_distribution$ ../bin/bin/g++ -std=c++0x -o test_help test_help.cpp Help.cpp
In file included from /home/ed/bin/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../../include/c++/4.7.0/memory:86:0,
                 from Help.h:4,
                 from test_help.cpp:3:
/home/ed/bin/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../../include/c++/4.7.0/bits/unique_ptr.h: In instantiation of 'void std::default_delete<_Tp>::operator()(_Tp*) const [with _Tp = Help::Impl]':
/home/ed/bin/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../../include/c++/4.7.0/bits/unique_ptr.h:245:4:   required from 'void std::unique_ptr<_Tp, _Dp>::reset(std::unique_ptr<_Tp, _Dp>::pointer) [with _Tp = Help::Impl; _Dp = std::default_delete<Help::Impl>; std::unique_ptr<_Tp, _Dp>::pointer = Help::Impl*]'
/home/ed/bin/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../../include/c++/4.7.0/bits/unique_ptr.h:169:32:   required from 'std::unique_ptr<_Tp, _Dp>::~unique_ptr() [with _Tp = Help::Impl; _Dp = std::default_delete<Help::Impl>]'
Help.h:6:7:   required from here
/home/ed/bin/lib/gcc/x86_64-unknown-linux-gnu/4.7.0/../../../../include/c++/4.7.0/bits/unique_ptr.h:63:14: error: invalid application of 'sizeof' to incomplete type 'Help::Impl'

This is a well known safety feature. I've tried to follow.

My problem is that if I put Help::Impl declaration in a header it would seem to obviate any advantage of pimpl. The class layout is visible to users. The definition is hidden but I could have done that with the Help class and private members. Also, including the declaration of Impl brings in new headers that I would have liked to keep separate.

What am I missing? What do folks put in an Impl declaration and where? Am I doing the Help dtor wrong? Argh!

Community
  • 1
  • 1
emsr
  • 13,551
  • 6
  • 45
  • 59
  • 5
    See also [GotW #101: Compilation Firewalls, Part 2](http://herbsutter.com/gotw/_101/) and [this related question](http://stackoverflow.com/q/8595471/636019). – ildjarn Jan 26 '12 at 16:49
  • 1
    Even though it's an old question, it's probably relevant to point out that, [as explained in cppreference](http://en.cppreference.com/w/cpp/language/pimpl), a PImpl implemented with `unique_ptr` should be wrapped in something like [`propagate_const`](http://en.cppreference.com/w/cpp/experimental/propagate_const) for complete correctness. – jdehesa Nov 18 '16 at 10:29
  • 1
    @jdehesa Thank you, I was looking at propagate_const as a solution to some API awkwardness. I almost wonder if unique_ptr is broken by default in this sense. It seems like propagate_const should be built-in or at least be the default. – emsr Nov 18 '16 at 17:52

2 Answers2

85

I believe that your test_help.cpp actually sees the ~Help() destructor that you declared default. In that destructor, the compiler tries to generate the unique_ptr destructor, too, but it needs the Impl declaration for that.

So if you move the destructor definition to the Help.cpp, this problem should be gone.

-- EDIT -- You can define the destructor to be default in the cpp file, too:

Help::~Help() = default;
xtofl
  • 38,207
  • 10
  • 95
  • 177
  • 9
    Okay, in the header I just *declared* the dtor for Help. Then in the implementation file I put Help::~Help() = default; Woot!!! Thanks. Everything works! – emsr Jan 26 '12 at 15:28
  • 4
    The smart pointers, including `unique_ptr` are required to work with an incomplete type. The "deleter" function is assigned at construction time, so that users of the pointer don't need to know how it is deleted. If this fixes the problem it would imply his `unique_ptr` implementation is broken. – edA-qa mort-ora-y Jan 26 '12 at 17:39
  • 5
    Appears my comment applies only to `shared_ptr`, so that `unique_ptr` is truly zero cost. Too bad there isn't an equivalent form of `unique_ptr` that can properly work with incomplete types. – edA-qa mort-ora-y Jan 26 '12 at 17:47
  • 8
    Putting ~Help() = default; in the header inlined the dtor. That was my block. – emsr Jan 26 '12 at 18:48
  • 1
    I find it interesting that that while =delete and =default look similar they are in fact rather different. The former is a declaration, the latter is a definition. – emsr Mar 02 '16 at 18:01
  • 1
    What is really weird is if you remove **constructor**, then code does not compile (same error) o_O… So, to make pImpl working both **constructor** and **destructor** must be implemented in cpp file – Dmytro Ovdiienko Apr 10 '19 at 15:41
  • @DmytroOvdiienko that is so weird that it deserves a proper question with the code that causes the behavior you mention. – xtofl Apr 10 '19 at 19:51
  • 1
    @xtofl https://stackoverflow.com/questions/55632570/pimpl-without-user-defined-constructor-using-stdunique-ptr – Dmytro Ovdiienko Apr 11 '19 at 12:31
8

Note this from unique_ptr definition:

std::unique_ptr may be constructed for an incomplete type T, such as to facilitate the use as a handle in the pImpl idiom. If the default deleter is used, T must be complete at the point in code where the deleter is invoked, which happens in the destructor, move assignment operator, and reset member function of std::unique_ptr.

parasrish
  • 3,084
  • 22
  • 28