Making all members of pimpl classes public

[Asfand Yar Qazi]

Hi,

I'm creating a library where several classes are intertwined rather tightly.
I'm thinking of making them all use pimpls, so that these circular
dependancies can be avoided easily, and I'm thinking of making all these pimpl
class declarations public. Reasoning is that since only the code within the
..cc file will need to ever access them, why protect them in ways that would
make access to them more difficult and obfuscated?

What are your thoughts on this?

 

[Phlip]

I'm creating a library where several classes are intertwined rather
tightly. I'm thinking of making them all use pimpls, so that these
circular dependancies can be avoided easily

Use Pimpls in an emergency, as a retrofit to a bad design.

If you are planning a design, write lots of unit tests, and read the
Refactoring books to help keep the design clean as it grows.

Then research the Dependency Inversion Principle, and use that to put in
just a few abstract interfaces. The necessities of your simple test cases
will force many of these interfaces to exist.

and I'm thinking of making all these pimpl class declarations public.
Reasoning is that since only the code within the .cc file will need to
ever access them, why protect them in ways that would make access to them
more difficult and obfuscated?

The rule is not "make all data members private". The rule is "don't make
primitive things globally public".

You should prefer to make everything as private as possible. Things inside
implementation files (".cc") are already private, so making them private: is
irrelevant.

A .cc file that mostly deals with its own contents, and has very few
dependencies out to other .cc files, is "coherent". The more coherent your
..cc file, the more useful and mobile it is to other .cc files. You can
change it easily without affecting other .cc files.

 

[Asfand Yar Qazi]

Asfand Yar Qazi wrote:




Use Pimpls in an emergency, as a retrofit to a bad design.

If you are planning a design, write lots of unit tests, and read the
Refactoring books to help keep the design clean as it grows.

Then research the Dependency Inversion Principle, and use that to put in
just a few abstract interfaces. The necessities of your simple test cases
will force many of these interfaces to exist.

I don't think I agree with that - the pimpl idiom easily separates interface
from implementation. It allows me to place just the interface in the .hh
file, without reference to a single data member. It easily allows
copy-on-write behaviour, and allows the interface file to remain unchanged
even if things underneath change radically.

Or is that what you meant?

 

[Aleksander Beluga]

I don't think I agree with that - the pimpl idiom easily separates
interface from implementation.

I guess you don't need pimpl to separate interface from implementation,
of course, if you're not making template classes.

 

[red floyd]

Pimpl is one of the standard patterns in GoF, although they call it the
"Bridge" pattern.

 

[werasm]

Asfand Yar Qazi wrote:

Use Pimpls in an emergency, as a retrofit to a bad design.

This seems naive to me. Pimpls used in conjunction with the Bridge
pattern are often required for good "dynamic" design. It is certainly
not only for emergency, or as result of refactoring.

If you are planning a design, write lots of unit tests, and read the
Refactoring books to help keep the design clean as it grows.

Specify your interface...

Then research the Dependency Inversion Principle, and use that to put in
just a few abstract interfaces. The necessities of your simple test cases
will force many of these interfaces to exist.

Hmmm, you are still required to construct your abstract classes
somewhere. I prefer making my clients dependent on non-abstract
classes. My pimpls are the abstract classes. Therefore:

Interface <non-abstract> has a pimpl <abstract>. The Pimpl (or bridge)
is then provided by a factory. The client has not need to provide, or
specify the concrete class.

You should prefer to make everything as private as possible. Things inside
implementation files (".cc") are already private, so making them private: is
irrelevant.

Yes, half true. What about the library implementor. Sometimes common
behaviour (not data) can be captured in the base class (template method
pattern). private methods are then called from the public method. On
the other hand, if this is not the case, and pimpl is implemented
correctly (i.e. it is abstract), then it only has methods callable from
the interface, which means there is not need to make anything private.

The other reason for using pimpl (compiler firewall) often has the
entire Impl class <then non-abstract> in the cpp file. In this case it
is fine to make everything public, as the Impl is often a privately
nested class, which makes it inaccessible to other classes anyway.

Regards,

Werner

 

[Richard Herring]

I guess you don't need pimpl to separate interface from implementation,
of course, if you're not making template classes.

Sometimes you do. Private declarations are conceptually part of the
implementation, not the interface, but the language syntax requires them
to appear in the interface definition. This introduces a coupling that's
not present in the ideal conceptual model which completely separates
interface and implementation, and this coupling causes recompilation of
everything that uses the interface whenever the private parts change. A
major practical reason for using the pimpl idiom is to reduce the
private part of the public interface to a single pointer, removing that
syntax-imposed coupling and reducing build times.

 

[Gavin Deane]

Sometimes you do. Private declarations are conceptually part of the
implementation, not the interface, but the language syntax requires them
to appear in the interface definition. This introduces a coupling that's
not present in the ideal conceptual model which completely separates
interface and implementation, and this coupling causes recompilation of
everything that uses the interface whenever the private parts change. A
major practical reason for using the pimpl idiom is to reduce the
private part of the public interface to a single pointer, removing that
syntax-imposed coupling and reducing build times.

It's not only when the private part of the class changes that coupling
causes recompilation. If the private part of my_class needs something
defined in "another_header.h" then, without pimpl, "my_class.h" must
include "another_header.h" or it won't compile (unless a forward
declaration is sufficient, which isn't always the case).

Anyone using my_class will obviously include "my_class.h". But then
they will also be indirectly including "another_header.h" even though
that header was only of interest to the private part of my_class. If
"another_header.h" changes, *all* users of my_class will be forced to
recompile even though they don't care what "another_header.h" is. And
even if the change doesn't even affect the private part of my_class.

Use a pimpl in my_class and users of the class are no longer affected
by any change to the "another_header.h".

Gavin Deane

 

[Phlip]

I don't think I agree with that - the pimpl idiom easily separates
interface
from implementation. It allows me to place just the interface in the .hh
file, without reference to a single data member. It easily allows
copy-on-write behaviour, and allows the interface file to remain unchanged
even if things underneath change radically.

Or is that what you meant?

Pimpl solves an emergency situation. If you have Foo.h, and it includes
many .h files, and it declares a huge class Foo, and if everyone uses Foo,
then each time you change any of those .h files, everything must recompile,
even things that do not directly use the changed things in those .h files.

The fastest and easiest fix replaces each .h file with a forward declare of
the class it defines, and replaces all of Foo's data members with a single,
bald pointer to FooImpl. Only define FooImpl in Foo.cpp, and forward every
method to it.

This is a ruthlessly effective fix because you needn't redesign Foo, or any
of its users, or any of its servants, to push in the fix. You only need a
few copies and pastes.

Now, in terms of the emergency, ask why Foo.h got so burdensome. Why do so
many things use it? Do they do too much? Why does it use so many servants?
Do they do too little?

If a class is well-designed, it will achieve many of Pimpl's benefits,
possibly including a bald pointer to an implementation object in a .cc
file. C++ works because the techniques that logically decouple modules
parallel the techniques that physically decouple them.

Ideally, if Foo is very important, its clients should access it through an
abstract interface; call this FooInf (but real code should think of a
better name). So now the clients cannot create a Foo because they are not
aware of it. If they want one, they must get it from a Foo Factory of some
kinds. This technique, "Construction Encapsulation", fully insulates Foo
clients from the concrete Foo, and its data members, just as rigorously as
the FooImpl would have. But the entire design is now more flexible.

 

[werasm]

So now the clients cannot create a Foo because they are not

aware of it. If they want one, they must get it from a Foo Factory of some
kinds. This technique, "Construction Encapsulation", fully insulates Foo
clients from the concrete Foo, and its data members, just as rigorously as
the FooImpl would have. But the entire design is now more flexible.

I mostly agree with you. I have mention this though:

I have seen applications where factories became the most used
(included) classes in an application. Every time new products came
along, the factory interface changed and everything needed to be
recompiled - especially because the client was made dependent on the
factory interface. Therefore, if you are implementing a library, hide
the factory interface from the client. This can be done very
conveniently by making the client dependent on a non-abstract
interfaces that makes use of the pimpl (being abstract), who's concrete
instance is provided by an opaque (not-client-visible) factory.

Regards,

Werner

 

[Ben Pope]

I mostly agree with you. I have mention this though:

I have seen applications where factories became the most used
(included) classes in an application. Every time new products came
along, the factory interface changed and everything needed to be
recompiled - especially because the client was made dependent on the
factory interface. Therefore, if you are implementing a library, hide
the factory interface from the client. This can be done very
conveniently by making the client dependent on a non-abstract
interfaces that makes use of the pimpl (being abstract), who's concrete
instance is provided by an opaque (not-client-visible) factory.

The whole point of a factory is to insulate users from "new products".
Sounds like a bad implementation of a factory to me.

Why does the interface of a factory have to change for each new product?
Why does the implementation even need to change? The whole point is
to push the management of ids and creating functions into the new
products, and away from centralised management.

Ben Pope

 

[Phlip]

I have seen applications where factories became the most used
(included) classes in an application.

For every pattern in the book /Design Patterns/, there is a big legacy app
out there written by those who didn't "get" some pattern, and abused it all
over the place.

There are probably more than three such abuse examples per pattern, these
days. ;-)

Every time new products came
along, the factory interface changed and everything needed to be
recompiled - especially because the client was made dependent on the
factory interface. Therefore, if you are implementing a library, hide
the factory interface from the client. This can be done very
conveniently by making the client dependent on a non-abstract
interfaces that makes use of the pimpl (being abstract), who's concrete
instance is provided by an opaque (not-client-visible) factory.

Right - you describe the emergency situation calling for a Pimpl fix.

The design violated "Construction Encapsulation". (Lots of unit tests would
have helped fix that, because test cases often construct fake objects and
pass them to the tested objects, who should not create their own objects.)

In a typical good design, data enters from the outside world as numbers, and
then factory-like things convert these to objects. Inside this "boundary
layer", objects just mix it up, constructing very few new objects, and
passing references around to each others' interfaces.

Ordinarily, too many switch statements are a design smell. In such
well-factored code, the joke is "too many 'if' statements are a design
smell".

 

[werasm]

The whole point of a factory is to insulate users from "new products".

Sounds like a bad implementation of a factory to me.

Ok, this may be a naive implementation, but more or less iaw. GOF. I
haven't got the book here, so bare with me (I'll be extra brief):

struct Factory
{
X* makeX() const;
Y* makeY() const;
Z* makeZ() const;
};

Ok, X, Y and Z is of course ABC's, right? therefore adding a new
implementation is fine - this won't cause the client to recompile. But
what about adding a new ABC?

struct Factory
{
W* makeW const;
X* makeX() const;
Y* makeY() const;
Z* makeZ() const;
};

Now interface of this factory has changed. All clients of Factory
requires recompilation, despite them not making use of W. How do you
overcome this? I'd love to see - always nice to be less naive
eventually...

Regards,

Werner

 

[werasm]

The design violated "Construction Encapsulation".

Not exactly sure what you mean here. Give me an example of a factory
not violating "Construction Encapsulation".

(Lots of unit tests would
have helped fix that, because test cases often construct fake objects and
pass them to the tested objects, who should not create their own objects.)

I may have a vague idea, but I'm not sure I see your point here too.
Small example please.

In a typical good design, data enters from the outside world as numbers, and
then factory-like things convert these to objects.

Factory like things? Does this mean the inner objects use (are exposed
to the interface) of these factory like things. How do these factory
like things look. Are they very concise - only responsible for creating
one type - ever, so that their interface can never change? If yes,
good. If no, problem.

Ordinarily, too many switch statements are a design smell. In such
well-factored code, the joke is "too many 'if' statements are a design
smell".

No ifs and no switches sound good, yes. Thats what I prefer.

Werner

 

[Phlip]

Not exactly sure what you mean here. Give me an example of a factory
not violating "Construction Encapsulation".

This is the answer to your other post:

struct Factory
{
X* makeX() const;
Y* makeY() const;
Z* makeZ() const;
};

That's not a factory, because calling aFactory.makeX() is the moral
equivalent of calling new X(). It's just as coupled.

This is a factory:

struct Factory
{
ABC* make(int type) const;
};

It essentially converts a type to a base class pointer. The calling code
never needs to know the class, only the type code.

Now this is unencapsulated construction:

void Foo()
{
ABC*p = new X;
...
}

This is partly encapsulated construction:

void Foo(int what)
{
ABC*p = someFactory.make(what);
...
}

And this is fully encapsulated construction:

void Foo(ABC & thing)
{
...
}

That's what it means - the clients of objects are not responsible for
creating their servant objects. They are only responsible for using them,
and Something Else creates them.

I may have a vague idea, but I'm not sure I see your point here too.
Small example please.

Suppose the code has three derived classes of ABC. Your X, Y, and Z.

Now suppose I write a unit test that creates a fourth derivative - MockABC.
It overrides ABC methods and puts test-ready things in them.

Which of my Foos is easiest to test? Which one is the easiest to pass a
MockABC into?

Now suppose I design my program by writing such tests. The easier classes
are to test, the more decoupled they become.

A test case:

TEST_(TestSuite, doFooThing)
{
MockABC aMock;
Foo(aMock);
CHECK_EQUAL(42, aMock.fooCalledMeWith());
}

Factory like things? Does this mean the inner objects use (are exposed
to the interface) of these factory like things. How do these factory
like things look. Are they very concise - only responsible for creating
one type - ever, so that their interface can never change? If yes,
good. If no, problem.

They look like simple factories. But you don't happen to call them that, or
"implement the Class Factory Pattern from the book Design Patterns". They
might look like the prototype pattern.

Consider this method:

void Foo(ABC & thing);

It has no Factory inside it. Something created the right Thing for it. It
doesn't care what.

Factory is a nice pattern, but you could conceivably write a program that
never couples its constructors to its calling code, yet has no recognizable
Factory in it.

 

[Ben Pope]

Ok, this may be a naive implementation, but more or less iaw. GOF. I
haven't got the book here, so bare with me (I'll be extra brief):

struct Factory
{
X* makeX() const;
Y* makeY() const;
Z* makeZ() const;
};

Ok, X, Y and Z is of course ABC's, right? therefore adding a new
implementation is fine - this won't cause the client to recompile. But
what about adding a new ABC?

struct Factory
{
W* makeW const;
X* makeX() const;
Y* makeY() const;
Z* makeZ() const;
};

Now interface of this factory has changed. All clients of Factory
requires recompilation, despite them not making use of W. How do you
overcome this? I'd love to see - always nice to be less naive
eventually...

Yep, I see your problem. You still need need to know your type at
runtime in order to instantiate it. You need to separate your type
information from your creation.

I don't have time to check this:


#include <string>
#include <map>

struct base{
virtual ~base() {}
};

struct derived : base {};

base* createBase() {
return new base;
}
base* createDerived() {
return new derived;
}

typedef base* (*creator)();

class Factory {
public:
void Register(std::string id, creator func) {
factory_[id] = func;
}
base* create(std::string id) {
return factory_[id]();
}
private:
std::map<std::string, creator> factory_;
};

int main() {
Factory factory;
factory.Register(std::string("base"), &createBase);
factory.Register(std::string("derived"), &createBase);

base* b = factory.create(std::string("base"));
return 0;
}


If you don't get it, I'm sure others can explain.

This is also covered in Modern C++ design (infinitely more elegant than
my example!)

Ben Pope

 

[Aleksander Beluga]

Pimpl is one of the standard patterns in GoF, although they call it the
"Bridge" pattern.

I believe that pImpl idiom is only C++ solution while DPs are usually
cross-language. Also, I believe that pImpl is a specific case of another
DP called Facade.

 

[Phlip]

I believe that pImpl idiom is only C++ solution while DPs are usually
cross-language. Also, I believe that pImpl is a specific case of another
DP called Facade.

DPs are logical design. Pimpl is physical design. That means you could take
the pimpl out, and all the client classes would behave the same. Except
they'd take longer to compile.

A refactor to a better design, such as via the Dependency Inversion
Principle, _should_ simplify all the clients.

 

[Alf P. Steinbach]

* Phlip:

DPs are logical design. Pimpl is physical design. That means you could take
the pimpl out, and all the client classes would behave the same. Except
they'd take longer to compile.

Nope.

pimpl removes dependencies on header files, some of which are usually
ill-behaved, and anyway, which introduce definitions not present with pimpl.

A refactor to a better design, such as via the Dependency Inversion
Principle, _should_ simplify all the clients.

On the contrary. ;-)

 

[Phlip]

Nope.

pimpl removes dependencies on header files, some of which are usually
ill-behaved, and anyway, which introduce definitions not present with
pimpl.

DPs are logical design. Pimpl is physical design. That means you could take
the pimpl out, and all the client classes would have exactly the same design
and contents. Except they'd take longer to compile, and would be exposed to
all the stray identifiers in the excess headers, that could change their own
identifier's meanings in subtle ways, if your C++ codebase was already
circling the drain anyway.

On the contrary. ;-)

Else why put it in?