pImpl idiom - what are the drawbacks of putting the private class implementation in cpp?

Question

What are the disadvantages of the following implementation of the pImpl idiom?

// widget.hpp

// Private implementation forward declaration
class WidgetPrivate;

// Public Interface 
class Widget
{
private:
    WidgetPrivate* mPrivate;

public:
    Widget();
    ~Widget();

    void SetWidth(int width);    
};

// widget.cpp
#include <some_library.hpp>

// Private Implementation
class WidgetPrivate
{
private:
    friend class Widget;

    SomeInternalType mInternalType;

    SetWidth(int width)
    {
        // Do something with some_library functions
    }
};

// Public Interface Implementation
Widget::Widget()
{
    mPrivate = new WidgetPrivate();
}

Widget::~Widget()
{
    delete mPrivate;
}

void Widget::SetWidth(int width)
{
    mPrivate->SetWidth(width);
}

I would prefer not to have seperate headers and sources for the private implementation part of the class because the code is essentially of the same class - should they not reside together then?

What alternatives to this version would be better?

Answer 1

I think there is no big difference. You can choose more convenient alternative.

But I have some other suggestions:

Usually in PIMPL I place implementation class declaration inside interface class:

class Widget
{
private:
   class WidgetPrivate;
...
};

This will prevent using of WidgetPrivate class outside of Widget class. So you dont need to declare Widget as friend of WidgetPrivate. And you can restrict access to implementation details of WidgetPrivate.

I recommend using of smart pointer. Change line:

WidgetPrivate* mPrivate;

to

std::unique_ptr<WidgetPrivate> mPrivate;

Using smart pointers you will not forget to delete member. And in case of exception thrown in constructor already created members will be always deleted.

My varian of PIMPL: // widget.hpp

// Public Interface 
class Widget
{
private:
    // Private implementation forward declaration
    class WidgetPrivate;

    std::unique_ptr<WidgetPrivate> mPrivate;

public:
    Widget();
    ~Widget();

    void SetWidth(int width);    
};

// widget.cpp
#include <some_library.hpp>

// Private Implementation
class Widget::WidgetPrivate
{
private:
    SomeInternalType mInternalType;

public:
    SetWidth(int width)
    {
        // Do something with some_library functions
    }
};

// Public Interface Implementation
Widget::Widget()
{
    mPrivate.reset(new WidgetPrivate());
}

Widget::~Widget()
{
}

void Widget::SetWidth(int width)
{
    mPrivate->SetWidth(width);
}

Answer 2

First, let's address the question of whether the private variables should reside with the class declaration. The private part of a class declaration is part of the implementation details of that class, rather than the interface exposed by that class. Any outside "user" of that class (whether it is another class, another module, or another program in the case of an API) will care only about the public part of your class, since that is the only thing that it can use.

Putting all the private variables directly into the class in a private section might look like it puts all relevant information in the same place (in the class declaration), but it turns out that not only are private member variables not relevant information, it also creates unnecessary and unwanted dependencies between your class' clients and what amounts to be implementation details.

If, for some reason, you need to add or remove a private member variable, or if you need to change its type (e.g. from float to double), then you modified the header file which represents the public interface to your class, and any user of that class needs to be recompiled. If you were exporting that class in a library, you would also break binary compatibility since, among other things, you probably changed the size of the class (sizeof(Widget) will now return a different value). When using a pImpl, you avoid those artificial dependencies and those compatibility problems by keeping implementation details where they belong, which is out of sight of your clients.

As you have guessed, there is however a tradeoff, which might or might not be important depending on your specific situation. The first tradeoff is that the class will lose some of its const-correctness. Your compiler will let you modify the content of your private structure within a method that is declared const, whereas it would have raised an error if it would have been a private member variable.

struct TestPriv {
    int a;
};

class Test {
public:
    Test();
    ~Test();

    void foobar() const;

private:
    TestPriv *m_d;
    int b;
};

Test::Test()
{
    m_d = new TestPriv;
    b = 0;
}

Test::~Test()
{
    delete m_d;
}

void Test::foobar() const
{
    m_d -> a = 5; // This is allowed even though the method is const
    b = 6;        // This will not compile (which is ok)
}

The second tradeoff is one of performance. For most applications, this will not be an issue. However, I have faced applications that would need to manipulate (create and delete) a very large number of small objects very frequently. In those rare extreme cases, the extra processing required to create an allocate an additional structure and to defer assignations will take a toll on your overall performance. Take note however that your average program certainly does not fall in that category, it is simply something that needs to be considered in some cases.

Answer 3

I do the same thing. It works fine for any simple application of the PImpl idiom. There is no strict rule that says that the private class has to be declared in its own header file and then defined in its own cpp file. When it is a private class (or set of functions) that is only relevant to the implementation of one particular cpp file, it makes sense to put that declaration + definition in the same cpp file. They make logical sense together.

What alternatives to this version would be better?

There is an alternative for when you need a more complex private implementation. For example, say you are using an external library which you don't want to expose in your headers (or want to make optional through conditional compilations), but that external library is complicated and requires that you write a bunch of wrapper classes or adaptors, and/or you might want to use that external library in similar way in different parts of your main project's implementation. Then, what you can do is create a separate folder for all that code. In that folder, you create headers and sources as you usually would (roughly 1 header == 1 class) and can use the external library at will (no PImpl'ing of anything). Then, the parts of your main project which need those facilities can simply include and use them only in the cpp files for implementation purposes. This is more or less the basic technique for any large wrappers (e.g., a renderer that wraps either OpenGL or Direct3D calls). In other words, it's PImpl on steroids.

In summary, if it's just for a single-serving use / wrapping of an external dependency, then the technique you showed is basically the way to go, i.e., keep it simple. But if the situation is more complicated, then you can apply the principle of PImpl (compilation firewall) but on a larger proportion (instead of a ext-lib-specific private class in a cpp file, you have a ext-lib-specific folder of source files and headers that you use only privately in the main parts of your library / project).