Pimpl Idiom

[Icosahedron]

I've been going through some old code trying to clean it up
and rearchitect it based on more modern C++ idioms. In the
old code I often used the Pimpl idiom on a class by class
basis, creating impl within a class as such:

a.h
class A {
...stuff...
struct AImpl;
AImpl* _impl;
};

a.cpp

struct AImpl {
...stuff...
};

After doing this in a number of classes, I thought it would
be "cool" to have a single Pimpl class that any class
wishing to define implementation specifics could inherit
from and then define a class specific Impl class as such:

pimpl.h

class Pimpl {
struct Impl;
boost::shared_ptr<Impl> _impl;
};

a.h

class A : public Pimpl {
... stuff ...
};

a.cpp

struct A::Impl {
... stuff ...
};

This compiled okay, but then I ran into an inheritance
problem (as many of you probably predicted just reading
this):

b.h

class B : public A, public Pimpl {
};

I could use virtual inheritance, but I want 2 impl classes,
one for A's implementation specific stuff and B's
implementation specific stuff. Of course, I thought I would
use templates to parameterize each one to give it a
different type so.

pimpl.h

template <typename T>
class Pimpl {
struct Impl;
boost::shared_ptr<Impl> _impl;
};

a.h

class A : public Pimpl<A> {
};

b.h

class B : public A, public Pimpl<B> {
};


Still, this has not quite gotten me where I want to be. I
would like to be able to have an A::_impl and a B::_impl
that I can distinguish inside of B based on the methods that
have to be called. The other is that, if possible, I wish
to refer to them as A::_impl rather than Pimpl<A>::_impl and
likewise for B.

Any suggestions? Is this whole thing just bad form? Should
I just stick with the original approach?

Jay

 

[Icosahedron]

I've been going through some old code trying to clean it up
and rearchitect it based on more modern C++ idioms. In the
old code I often used the Pimpl idiom on a class by class
basis, creating impl within a class as such:

a.h
class A {
...stuff...
struct AImpl;
AImpl* _impl;
};

a.cpp

struct AImpl {
...stuff...
};

Here I meant A::AImpl of course.

The idea behind "unifying" all the different Pimpls is that
a number of the impl classes must talk to each other within
the module. Rather than have friend declarations scattered
all throughout the heirarchy, I thought it would be better
to have a single Pimpl and make it a friend of itself, so
that the classes could interact within the module freely.

pimpl.h

class Pimpl {
struct Impl;
boost::shared_ptr<Impl> _impl;
};

a.h

class A : public Pimpl {
... stuff ...
};

a.cpp

struct A::Impl {
... stuff ...
};

This compiled okay, but then I ran into an inheritance
problem (as many of you probably predicted just reading
this):

b.h

class B : public A, public Pimpl {
};

I could use virtual inheritance, but I want 2 impl classes,
one for A's implementation specific stuff and B's
implementation specific stuff. Of course, I thought I would
use templates to parameterize each one to give it a
different type so.

pimpl.h

template <typename T>
class Pimpl {
struct Impl;
boost::shared_ptr<Impl> _impl;
};

a.h

class A : public Pimpl<A> {
};

b.h

class B : public A, public Pimpl<B> {
};


Still, this has not quite gotten me where I want to be. I
would like to be able to have an A::_impl and a B::_impl
that I can distinguish inside of B based on the methods that
have to be called. The other is that, if possible, I wish
to refer to them as A::_impl rather than Pimpl<A>::_impl and
likewise for B.

Of course, having two _impl members is impossible, but
having two GetImpl functions, each one with its own return
value would be possible, each addressed by its class prefix

Any suggestions? Is this whole thing just bad form? Should
I just stick with the original approach?

Again, the above question still remains in effect.

Jay

 

[David Rubin]

I've been going through some old code trying to clean it up
and rearchitect it based on more modern C++ idioms. In the
old code I often used the Pimpl idiom on a class by class
basis, creating impl within a class as such:
[snip]
class A {
...stuff...
struct AImpl;
AImpl* _impl;
};
[snip]
After doing this in a number of classes, I thought it would
be "cool" to have a single Pimpl class that any class
wishing to define implementation specifics could inherit
from and then define a class specific Impl class as such:
[snip]
class Pimpl {
struct Impl;
boost::shared_ptr<Impl> _impl;
};
[snip]
class A : public Pimpl {
... stuff ...
};

[snip - problem with multiple inheritance and general confusion]

Any suggestions? Is this whole thing just bad form? Should
I just stick with the original approach?

The thing is, you don't want to inherit an implementation interface, you
want a *pointer* to an implementation object. This allows you to keep
the interface and implementation separate, alleviating compile time
dependencies, and improving encapsulation. So, your entire question is
ill concieved, IMO. Stick to the original plan.

/david

 

[Icosahedron]

[snip - problem with multiple inheritance and general confusion]

The thing is, you don't want to inherit an implementation interface, you
want a *pointer* to an implementation object. This allows you to keep
the interface and implementation separate, alleviating compile time
dependencies, and improving encapsulation. So, your entire question is
ill concieved, IMO. Stick to the original plan.

A good point. The reason I chose to do this (and not explained in the
original post) was that in a suite of classes I am using, often one class'
impl must gain
access to another classes impl, but might only have the reference to the
envelope class.

class A {
private:
struct Impl;
Impl* _impl;
friend class C;
};

class C {
public:
void DoSomething(A* a);
private:
struct Impl;
Impl* _impl;
};

where C might need to get something out of A's impl or call a method on it.

C:oSomething(A* a)
{
a->_impl->DoImplSpecificStuff();
}

How would you design this so that the friend statements aren't necessary?
Since the impls are hidden anyways, making them public really doesn't expose
any functionality, so I could do it that way, but of course that's not
really
elegant. I appreciate the feedback.

Thanks,

Jay

 

[Jack Walker]

[snip - problem with multiple inheritance and general confusion]

Any suggestions? Is this whole thing just bad form? Should
I just stick with the original approach?

The thing is, you don't want to inherit an implementation interface, you
want a *pointer* to an implementation object. This allows you to keep
the interface and implementation separate, alleviating compile time
dependencies, and improving encapsulation. So, your entire question is
ill concieved, IMO. Stick to the original plan.

A good point. The reason I chose to do this (and not explained in the
original post) was that in a suite of classes I am using, often one class'
impl must gain
access to another classes impl, but might only have the reference to the
envelope class.

class A {
private:
struct Impl;
Impl* _impl;
friend class C;
};

class C {
public:
void DoSomething(A* a);
private:
struct Impl;
Impl* _impl;
};

where C might need to get something out of A's impl or call a method on it.

C:oSomething(A* a)
{
a->_impl->DoImplSpecificStuff();
}

How would you design this so that the friend statements aren't necessary?
Since the impls are hidden anyways, making them public really doesn't expose
any functionality, so I could do it that way, but of course that's not
really
elegant. I appreciate the feedback.

Thanks,

Jay

This may be telling you that the pimpl idiom is not well suited to your application.

Jack

 

[Icosahedron]

I've been going through some old code trying to clean it up
and rearchitect it based on more modern C++ idioms. In the
old code I often used the Pimpl idiom on a class by class
basis, creating impl within a class as such:

[snip]

Still, this has not quite gotten me where I want to be. I
would like to be able to have an A::_impl and a B::_impl
that I can distinguish inside of B based on the methods that
have to be called. The other is that, if possible, I wish
to refer to them as A::_impl rather than Pimpl<A>::_impl and
likewise for B.

Any suggestions? Is this whole thing just bad form? Should
I just stick with the original approach?

FWIW, I finally got what I wanted, more or less. Here's a sample showing
what I've done so far. Amazing how simple these things always seem to boil
down to.

#include <iostream>

using namespace std;

template <typename T>
struct Impl;

class C;

class A {
struct Impl<A>* _impl;
template <class T> friend class Impl;
public:
A();
};

class B : public A {
struct Impl<B>* _impl;
template <class T> friend class Impl;
public:
B();
void Print(C*);

};

class C {
struct Impl<C>* _impl;
template <class T> friend class Impl;
public:
C();
};


struct Impl<A> {
A* that;
Impl(A *a) { that = a; }
void Print(void) { cout << "In Impl<A>:rint" << endl; }
};

struct Impl<B> {
B* that;
Impl(B *b) { that = b; }
void Print(C* c);
};

struct Impl<C> {
C* that;
Impl(C *c) { that = c; }
void Print(void) { cout << "In Impl<C>:rint" << endl; }
};

A::A() : _impl(new Impl<A>(this)) {}
B::B() : _impl(new Impl<B>(this)) {}
C::C() : _impl(new Impl<C>(this)) {}

void Impl<B>:rint(C* c)
{
cout << "In Impl<B>:rint" << endl;
that->A::_impl->Print(); // shows accessing a base class' impl
c->_impl->Print(); // shows accessing another class' impl
}

void B:rint(C* c)
{
cout << "In B:rint" << endl;
_impl->Print(c); // forward to impl class
}

int main(void)
{
C c;
B b;
b.Print(&c);
}

As you may see, I dropped the Pimpl being inherited. I don't know why I was
thinking it, but I was doing that to allow for the automatic declaration of
the Impl class, which would force the implementer to define a nested Impl
class. Unfortunately, there is no way to extend friendship between these
classes to the nested Impl classes or the main classes well.

As for the Pimpl idiom being correct or not for this application, I chose it
because it is a cross platform application wherein I wanted the
implementation specific details to be kept out of the interface classes
(though truthfully the interface classes contain data common to all
platforms too, so they are not pure interface classes). I could have just
defined the methods themselves for the classes on a platform by platform
basis (each implementation for A could have been in separate directories for
instance), and just kept the data in the impl structures, which would have
worked I believe, and still given me the same opacity with regards to the
specifics, but I wanted the freedom to add methods instead of just helper
functions, though the distinction is pretty moot.

The main reason I wanted this was that in my particular application, a
renderer, the impl classes need access to details in other impl classes
(such as texture id from the TextureImpl for setting the current texture in
the CameraImpl class), and I didn't want to have to have friends defined for
every particular combination. (The friends were required because to get to
the _impl member of another class required that you have access to the
private members of that class.) I could have done it one of several ways I
believe:

1) make the _impl member public - this means anyone can access it, but
without the interface, it's pretty hard to do anything with it. This also
could be done also with a GetImpl member function. Opaque, but not guarded
from modification well.

2) friends. Of course there's the obvious friend class declaration, but as
mentioned, for every shader for instance, the Camera::Impl would have to be
put in, and likewise each Shader::Impl would have to be listed in Camera.
And that has to be in the main header file, not the implementation files.
Or, I could do as I did and make all the Impl classes friends by default
with one line. Not perfect, but done only once and still get the benefits
of hidden implementation and access restriction from the outside.

Along with #2, I am considering this is being designed to allow extension by
other developers (with their classes being first class members of the
library as well rather than some hacked in extension mechanism), I wanted
some way for them to add to the library without having to add their classes
as friends to the main classes.

So, the main mechanism is that you have to have an Impl<T> member named
_impl and a friend declaration "template <class T> friend class Impl;" that
will automatically define other Impl<T> classes in the module as friends of
the class. I keep the Impl class definition in the Render namespace, to
avoid any potential conflicts and to preserve some semblance of module
integrity.

Well, that's the short of it. Please feel free to comment, as I do read
them, if I don't respond to each one. Thanks again.

Jay

P.S. The above code compiles fine on GCC 3.2.3

 

[Jeff]

Hi Jay,

... in a suite of classes I am using, often one class's impl must gain
access to another class's impl, but might only have the reference to the
envelope class.
[e.g.]
C:oSomething(A* a)
{ a->_impl->DoImplSpecificStuff(); }

How would you design this so that the friend statements aren't necessary?

This is a design issue. Imagine what you're saying: you're
encapsulating stuff by making it private in class A, and then you're
specifically breaking (or at least dramatically reducing) the
encapsulation by having class C use private member functions in class
A. The classes are now more tightly coupled ... a change in class A
may necessitate a change in class C (suppose A:oImplSpecificStuff()
disappears, etc) If class C wants to be able to call a member
function in class A, it should be public.

Optionally, you could define a separate friend class of A that has the
helper function in it, and have both A and C call that (I believe this
is better than having C be a friend of A, since C itself doesn't
really need to be a friend to /all/ of A, it just needs a specific
function ... sorry, don't have time to really explain myself on this,
and I'm kind of shooting from the hip, it might be a crappy idea).

Anyhow, keep thinking about the design. I don't believe that
"friendship" is the right answer here, and you can probably come up
with something better that is more encapsulated. Good luck!

Jeff

 

[Icosahedron]

This is a design issue. Imagine what you're saying: you're

encapsulating stuff by making it private in class A, and then you're
specifically breaking (or at least dramatically reducing) the
encapsulation by having class C use private member functions in class
A. The classes are now more tightly coupled ... a change in class A
may necessitate a change in class C (suppose A:oImplSpecificStuff()
disappears, etc) If class C wants to be able to call a member
function in class A, it should be public.

True. Actually, the only thing I need is access to the _impl class member
so I can call impl specific functions from another impl specific class in
the module, but I don't know of another mechanism to grant that.

Optionally, you could define a separate friend class of A that has the
helper function in it, and have both A and C call that (I believe this
is better than having C be a friend of A, since C itself doesn't
really need to be a friend to /all/ of A, it just needs a specific
function ... sorry, don't have time to really explain myself on this,
and I'm kind of shooting from the hip, it might be a crappy idea).

Anyhow, keep thinking about the design. I don't believe that
"friendship" is the right answer here, and you can probably come up
with something better that is more encapsulated. Good luck!

Thanks. I see what you're saying. I posted a general response to this.
You are right about the classes being tightly coupled, and you are right
about this breaking encapsulation, but at least in my mind it does so within
the module, and so really isn't breaking modular encapsulation.

As for the A->B->C link, I see what you're saying. I think that makes more
complication within a module where things are tightly coupled by nature, but
it is a good idea for keeping encapsulation.

I'll have to rethink what I've got and perhaps come up with something else.
I originally wrote a long essay about why what I was doing was the right
thing, and I still think it is the easiest within the module, but perhaps I
can rethink what I'm doing internally to make it not necessary.

Jay