Pimpl idiom with inheritance

Question

I want to use pimpl idiom with inheritance.

Here is the base public class and its implementation class:

class A
{
    public:
      A(){pAImpl = new AImpl;};
      void foo(){pAImpl->foo();};
    private:
      AImpl* pAImpl;  
};
class AImpl
{
    public:
      void foo(){/*do something*/};
};

And I want to be able to create the derived public class with its implementation class:

class B : public A
{
    public:
      void bar(){pAImpl->bar();};    // Can't do! pAimpl is A's private.
};        

class BImpl : public AImpl
{
    public:
      void bar(){/*do something else*/};
};

But I can't use pAimpl in B because it is A's private.

So I see some ways to solve it:

1. Create BImpl* pBImpl member in B, and pass it to A with additional A constructor, A(AImpl*).
2. Change pAImpl to be protected (or add a Get function), and use it in B.
3. B shouldn't inherit from A. Create BImpl* pBImpl member in B, and create foo() and bar() in B, that will use pBImpl.
4. Any other way?

What should I choose?

Answer 1

class A
{
    public:
      A(bool DoNew = true){
        if(DoNew)
          pAImpl = new AImpl;
      };
      void foo(){pAImpl->foo();};
    protected:
      void SetpAImpl(AImpl* pImpl) {pAImpl = pImpl;};
    private:
      AImpl* pAImpl;  
};
class AImpl
{
    public:
      void foo(){/*do something*/};
};

class B : public A
{
    public:
      B() : A(false){
          pBImpl = new BImpl;
          SetpAImpl(pBImpl);
      };
      void bar(){pBImpl->bar();};    
    private:
      BImpl* pBImpl;  
};        

class BImpl : public AImpl
{
    public:
      void bar(){/*do something else*/};
};

Answer 2

I think the best way from a purely object-oriented-theoretical perspective is to not make BImpl inherit from AImpl (is that what you meant in option 3?). However, having BImpl derive from AImpl (and passing the desired impl to a constructor of A) is OK as well, provided that the pimpl member variable is const. It doesn't really matter whether you use a get functions or directly access the variable from derived classes, unless you want to enforce const-correctness on the derived classes. Letting derived classes change pimpl isn't a good idea - they could wreck all of A's initialisation - and nor is letting the base class change it a good idea. Consider this extension to your example:

class A
{
protected:
   struct AImpl {void foo(); /*...*/};
   A(AImpl * impl): pimpl(impl) {}
   AImpl * GetImpl() { return pimpl; }
   const AImpl * GetImpl() const { return pimpl; }
private:
   AImpl * pimpl;
public:
   void foo() {pImpl->foo();}


   friend void swap(A&, A&);
};

void swap(A & a1, A & a2)
{
   using std::swap;
   swap(a1.pimpl, a2.pimpl);
}

class B: public A
{
protected:
   struct BImpl: public AImpl {void bar();};
public:
   void bar(){static_cast<BImpl *>(GetImpl())->bar();}
   B(): A(new BImpl()) {}

};

class C: public A
{
protected:
   struct CImpl: public AImpl {void baz();};
public:
   void baz(){static_cast<CImpl *>(GetImpl())->baz();}
   C(): A(new CImpl()) {}
};

int main()
{
   B b;
   C c;
   swap(b, c); //calls swap(A&, A&)
   //This is now a bad situation - B.pimpl is a CImpl *, and C.pimpl is a BImpl *!
   //Consider:
   b.bar(); 
   //If BImpl and CImpl weren't derived from AImpl, then this wouldn't happen.
   //You could have b's BImpl being out of sync with its AImpl, though.
}

Although you might not have a swap() function, you can easily conceive of similar problems occurring, particularly if A is assignable, whether by accident or intention. It's a somewhat subtle violation of the Liskov substitutability principle. The solutions are to either:

1. Don't change the pimpl members after construction. Declare them to be AImpl * const pimpl. Then, the derived constructors can pass an appropriate type and the rest of the derived class can downcast confidently. However, then you can't e.g. do non-throwing swaps, assignments, or copy-on-write, because these techniques require that you can change the pimpl member. However however, you're probably not really intending to do these things if you have an inheritance hierarchy.

2. Have unrelated (and dumb) AImpl and BImpl classes for A and B's private variables, respectively. If B wants to do something to A, then use A's public or protected interface. This also preserves the most common reason to using pimpl: being able to hide the definition of AImpl away in a cpp file that derived classes can't use, so half your program doesn't need to recompile when A's implementation changes.

Answer 3

As stefan.ciobaca said, if you really wanted A to be extendable, you'd want pAImpl to be protected.

However, your definition in B of void bar(){pAImpl->bar();}; seems odd, as bar is a method on BImpl and not AImpl.

There are at least three easy alternatives that would avoid that issue:

1. Your alternative (3).
2. A variation on (3) in which BImpl extends AImpl (inheriting the existing implementation of foo rather than defining another), BImpl defines bar, and B uses its private BImpl* pBImpl to access both.
3. Delegation, in which B holds private pointers to each of AImpl and BImpl and forwards each of foo and bar to the appropriate implementer.

Answer 4

I would do (1) because A's privates are or no business for B.

Actually I would not pass it to A as you suggest, because A makes its own in A::A(). Calling pApimpl->whatever() from Bis also not appropriate (private means private).

Answer 5

The correct way is to do (2).

In general, you should probably consider to make all you member variables protected by default instead of private.

The reason most programmers choose private is that they don't think about others who want to derive from their class and most introductory C++ manuals teach this style, in the sense that all the examples use private.

EDIT

Code duplication and memory allocation are undesired side-effects of using the pimp design pattern and cannot be avoided to my knowledge.

If you need to have Bimpl inherit Aimpl and you want to expose a consistent interface to them through A and B, B would also need to inherit A.

One thing you can do to simplify things in this scenario is to have B inherit from A and only change the contructor such that B::B(...) {} creates a Bimpl, and add dispatches for all methods of Bimpl that are not in Aimpl.