views:

253

answers:

7

Is there any reason for the implementation class as used in the pimpl idiom to have any private members at all? The only reason I can really think of is to protect yourself from yourself -- i.e. the private members serve to enforce some kind of contract between the class and the user, and in this case the class and the user are rather intimately related, so it seems unnecessary.

+2  A: 

Why shouldn't it have private members? Just because you're defining one interface as PIMPL doesn't mean that you will at no other time want to use the class.

It's still a class. Data should probably be private or protected. Operations on data that will never be accessible to the public, private or protected. Operations that you might wish to expose, protected, or public.

Liz Albin
My main reason for not wanting private members is sheer laziness -- I don't want to provide accessors. I realize there may also be members that should be private and don't require accessors as well.
xzqx
It is really a "private" class - accessible only within one cpp file. A C-style struct is just fine for the purpose in most cases.
Nemanja Trifunovic
Nemanja: This assumes the outside and inside are written at the same time. I've added pimpl after the fact to existing implementations.
Steven Sudit
@Steven. Not sure I understand your point. If you change your existing class to use pImpl, you keep the interface of the original class as-is, and move all the private data to a struct that is visible only within the implementation cpp file, right?
Nemanja Trifunovic
@xzqx: Why do you need to write so many accessors? Is all your data actually accessible for public use? Why aren't operations internal to the class?
Liz Albin
@Nemanja: The use case I'm talking about is when the impl class already exists and isn't even under your control, so you write the pimpl class to hide the impl away. The public interface of the pimpl class will be *similar to* that of the impl, but it may well be a subset or otherwise hide some aspects.
Steven Sudit
@Liz good point.
xzqx
@Steven: Wouldn't your use case be the Adapter or Bridge pattern, rather than Pimpl?
Emile Cormier
@Emile: Exactly what I thought.
Nemanja Trifunovic
Based on http://www.gotw.ca/publications/mill05.htm, I think what I'm describing qualifies as pimpl.
Steven Sudit
Perhaps there is a gray area between "pure" Pimpl and the Adapter/Bridge patterns?
Emile Cormier
@Emile: Perhaps. Or maybe they're somewhat orthogonal, with pimpl being a language-specific technique that can be used to implement the more general Adapter/Bridge pattern (but also to implement other patterns).
Steven Sudit
+3  A: 

In theory a pimpl class is still just a class like any other. That it is a concrete implementation of an interface doesn't mean that other code isn't a client of the pimpl class itself.

That said, in practice I have found that pimpl classes tend to be much closer to structs with some member functions rather than full fledged objects, and have less need to separate the interface from the implementation.

John Dibling
+1  A: 

Depends on your implementation of pImpl - specifically where you enforce the class invariant, but in general I see no need for the impl part to have protected/private members. In fact, I usually declare it as a struct.

Nemanja Trifunovic
A: 

(I misunderstood the question, so I'm changing my answer.)

The implementing class, which is pointed to by the class with the pimpl, should be a regular class, with just as much reason to hide private details as any other. In fact, it's often a pre-existing class, with the pimpl layer added later to break dependencies and perhaps simplify the interface a bit.

Steven Sudit
The pre-existing class is the "outer" one, not the "impl" part.
Nemanja Trifunovic
I've seen it go both ways. Sometimes pimpl is a way to put a consistent interface over implementations that are subject to change. Other times, pimpl is a way to to hide away the implementation completely, breaking compile-time dependencies. In fact, both can be goals.
Steven Sudit
+1  A: 

The only reason I can really think of is to protect yourself from yourself

Which is why "private" and "protected" are there in the first place. Of course you should use them in your implementation - the only time I would not is if the implementation has no behaviour (in which case it isn't really an implementation).

anon
A: 

Because private means better data encapsulation.

It seems silly, I know, but we have a way of defining interfaces at work that is very simple:

class Interface;

class Concrete { public: .... private: Interface* m_interface; }

class ConcreteFoo: public Interface {};

The main advantage: you can swap for another ConcreteBar at any moment. It is, in fact, a combination of Pimpl and Strategy pattern, and the constructor of Concrete will call a Factory that will be responsible to serve the effective object.

If you cannot think of a second way of implementing the heart, just encapsulate it in a class. This way if you later have to refactor, you'll just have to compose an abstract Interface with the exact same set of methods, change a few pointers (and the constructor) and you'll be all right ;)

Matthieu M.
+4  A: 

I think people are confusing the Pimpl idiom with Adapter/Bridge/Strategy patterns. Idioms are specific to a language. Patterns can apply to many languages.

The Pimpl idiom was devised to address the following problem in C++: Private members of a class are visible in the class declaration, which adds unnecessary #include dependencies to the user of the class. This idiom is also known as compiler firewall.

If the implementation is written directly in the outer class's corresponding *.cpp file, and is not accessible outside the module, then I think its perfectly fine to simply use a struct for the Pimpl class. To further re-enforce the idea that implementations are not meant to be directly re-used, I define them as a private inner struct:

// foo.h
class Foo : boost::noncopyable
{
public:
   ...

private:
   struct Impl;
   boost::scoped_ptr<Impl> impl_;
};

// foo.cpp
struct Foo::Impl
{
   // Impl method and member definitions
};

// Foo method definitions

As soon as there's a header file for the implementation class, I think we are no longer talking about the Pimpl idiom. We are rather talking about Adapter, Bridge, Strategy, interface classes, etc...

Just my 2 cents.

Emile Cormier