Pimpl idiom as template base class

Question

I'm currently researching the pimpl idiom and there are very nice tutorials how it could be implement (e.g. here). But i have never seen it implemented as a base template class like this:

#ifndef PIMPL_H
#define PIMPL_H

template <class T>
class Pimpl
{
public:
    explicit Pimpl();
    explicit Pimpl(T *ptr);
    virtual ~Pimpl() = 0;

    Pimpl(const Pimpl<T> &other);
    Pimpl &operator=(const Pimpl<T> &other);

protected:
    T *d_ptr;
};

template<class T>
Pimpl<T>::Pimpl() : d_ptr(new T)
{

}

template<class T>
Pimpl<T>::Pimpl(T *ptr) : d_ptr(ptr)
{

}

template<class T>
Pimpl<T>::~Pimpl()
{
    delete d_ptr;
    d_ptr = 0;
}

template<class T>
Pimpl<T>::Pimpl(const Pimpl<T> &other) : d_ptr(new T(*other.d_ptr))
{

}

template<class T>
Pimpl<T> &Pimpl<T>::operator=(const Pimpl<T> &other)
{
    if (this != &other) {
        delete d_ptr;
        d_ptr = new T(*other.d_ptr);
    }

    return *this;
}

#endif // PIMPL_H

Which then could be used in any class you like to pimpl:

#ifndef OBJECT_H
#define OBJECT_H

#include "pimpl.h"

class ObjectPrivate;

class Object : public Pimpl<ObjectPrivate>
{
public:
    Object();
    virtual ~Object();

    /* ... */
};

#endif // OBJECT_H

Currently i'm using it in a small example project (build as a shared library) and the only problem i had, was that MSVC warns about the missing destructor for ObjectPrivate (see C4150). This warning only occurs, because ObjectPrivate is forward declared and therefore not visible to the delete operator in Pimpl::~Pimpl() at compile time.

Does anyone see any sort of problems with this approach? :-)

---

So there is a now a final version based on the discussion below on GitHub (big thanks to StoryTeller). The repository also contains a simple usage example.

Answer 1

Yes, there are several problems, as I see it.

1. Your class is essentially a mixin. It's not about dynamic polymorphism, so no-one is ever going to call delete on a pointer to Pimpl<ObjectPrivate>. Drop the virtual destructor. It's introducing overhead that's never going to be required. What you want is static polymorphism only.

2. You allocate the object with new and release with delete. I won't use your template, because that allocation scheme isn't always appropriate in my applications. You must give a way to customize the allocation scheme in order to make your class actually useful.

3. Your assignment operator isn't exception safe. If the constructor for T throws, you lose the previously saved data. IMO it's better in this case to use the copy and swap idiom.

The solution to (1) and (2) is to add more template parameters, where the first is for the CRTP. This will allow you to push operations you aren't aware of how to do, onto the class that inherits your mixin. It can override them by defining its own make, unmake and clone. And those will all be bound statically.

template <class Handle, class Impl>
class Pimpl
{
private:
    Impl* _make() const
    { return ((Handle const*)this)->make(); }

    void _unmake(Impl *p) const
    { ((Handle const*)this)->unmake(p); }

    Impl* _clone(Impl *p) const
    { return ((Handle const*)this)->clone(p); }

    void swap(Pimpl &other) {
        Impl *temp = d_ptr;
        d_ptr = other.d_ptr;
        other.d_ptr = temp;
    }

public:
    explicit Pimpl();
            ~Pimpl();

    Pimpl(const Pimpl &other);
    Pimpl &operator=(const Pimpl &other);

    // fall-backs
    static Impl* make()          { return new Impl; }
    static void  unmake(Impl* p) { delete p; }
    static Impl* clone(Impl* p)  { return new Impl(*p); }

protected:

    Impl *d_ptr;
};

template<class Handle, class Impl>
Pimpl<Handle, Impl>::Pimpl() :
  d_ptr(_make())
{

}

template<class Handle, class Impl>
Pimpl<Handle, Impl>::~Pimpl()
{
    _unmake(d_ptr);
    d_ptr = 0;
}

template<class Handle, class Impl>
Pimpl<Handle, Impl>::Pimpl(const Pimpl &other) :
  d_ptr(_clone(other.d_ptr))
{

}

template<class Handle, class Impl>
Pimpl<Handle, Impl> &Pimpl<Handle, Impl>::operator=(const Pimpl &other)
{
    Pimpl copy(other);
    swap(copy);

    return *this;
}

Live Example

Now your header can compile cleanly. So long as the destructor for Object isn't defined inline. When it's inline the compiler must instantiate the destructor of the template wherever object.h is included.

If it's defined in a cpp file, after the definition of ObjectPrivate, then the instantiation of ~Pimpl will see the full definition of the private parts.

Further ideas for improvement:

1. Make the special members protected. It's only the derived Handle class that's supposed to call them, after all.

2. Add support for move semantics.

Answer 2

But i have never seen it implemented as a base template class

Vladimir Batov did it: https://github.com/yet-another-user/pimpl

Does anyone see any sort of problems with this approach?

You need to take the warning seriously. If your ObjectPrivate actually has a non-trivial destructor (which is as easy as containing a std::string member), you have undefined behavior, and the destructor probably won't get called.

This typically suggests that for some reason, the destructor is instantiated in the wrong place. Make sure that all definitions of all constructors and destructors of the derived class are placed after the full definition of ObjectPrivate. This includes the implicit copy and move constructors, which are probably what triggers the warning in your example code. Yes, this means you have to explicitly declare these special functions (and as a consequence, also the copy and move assignment operators if you want them), but at least you can use a defaulted definition.

I don't know if Vlad's library has the same problem.

Also, nulling out pointers in a destructor is pointless and will probably just get optimized away by some modern compilers.