Pimpl idiom and internal object collaboration without friend declaration

Question

I'm implementing several classes using the pimpl idiom and am coming across some design issues.

Firstly, I've always seen pimpl done like this

class Object
{
public:
    Visible();
    ~Visible();
 .. etc ..
private:
    class ObjectImpl *_pimpl;
};

I have several classes which use this approach and my problem is that several of these classes need access to each others implementation details but the _pimpl pointer is delcared private.

Can anyone see the downside of declaring the _pimpl public. Obviously, if it's public then someone may accidentally (or deliberately) reassign it. (I'm ignoring the fact that "private" could be #defined as "public" and grant access anyway. If you do this then you deserve what you get).

I appreciate that my design may be flawed and would welcome any comments along those lines also.

I'm really loathe to use friends and am not sure they'll even help as you can't forward declare the Object::ObjectImpl without fully defining Object.

i.e.

 ...
 private:
    class ObjectImpl *_pimpl;
    friend class OtherObject::OtherObjectImpl; // this needs a fully qualified OtherObject
};

Thx Mark.

* UPDATE - More detail **

I have two classes, one called Command, the other called Results. I have methods on Command which return a vector of Results.

Both Command and Results use the pimpl idiom. I want the interface to Results to be as small as possible.

class Command
{
public:
    void getResults( std::vector< Results > & results );
    void prepareResults( std::vector< Results > & results );
private:
    class CommandImpl *_pimpl;
};

class Results
{
public:
    class ResultsImpl;

    Results( ResultsImpl * pimpl ) :
        _pimpl( impl )
    {
    }

private
    ResultsImpl *_pimpl;
};

Now in Command::getResults(). I inject the ResultsImpl into the Results. in Command::prepareResults() I need access to the ResultsImpl.

M.

Nicknickel

A bit more detail is needed, I think. – Meanly

Answer 1

I doubt there is a good reason to make the implementation public: you can always expose the implementation's functionality using public methods:

class Object
{
public:
   Object();
  ~Object();

  int GetImplementationDetail();

private:
  std::unique_ptr< ObjectImpl > _pimpl;
};

int Object::GetImplementationDetail()
{
  return pimpl->GetImplementationDetail();
}

Apart from that a class should be responsible for one thing, one thing only, and should have the bare minimum of dependencies to other classes; if you think other classes should be able to access your Object's pimpl then your design is likely flawed.

edit following the author's update: although your example is still rather vague (or at least I cannot tell the full intent of it), you seem to be misinterpreting the idiom and now try to apply it to a case where it is not usefull. As others pointed out, the 'P' stands for private. Your results class doesn't have much private implementation since all of it is public. So either try to use what I mention above and do not 'inject' anything, or just get rid of the pimpl all together and use just the Result class. If your Result's class interface should be so small that it's nothing but a pointer to another class it doesn't seem to be of much use in this case.

Fagin

+1: nice touch with the unique_ptr :) Note that could provide a "custom deleter" (instead of defining the destructor) even though the net effect would be quite similar. – Damian

@sbi: automatic memory handling with correct destructor and move constructor/assignment for free – Damian

@sbi: shared_ptr is often (ill-)used for Pimpl, without due consideration about the shared semantics it implies :/ – Damian

@Matthieu: I am not convinced that implicit move semantics is that much better than implicit share semantics. What am I missing? – Tropopause

@sbi: when an object is "moved" there is still a unique owner (as the name of unique_ptr implies), thus we are still talking about "value objects", and only optimize its passing around. On the other hand, shared ownership means that several "reference objects" share the same underlying data, and thus changing to one are reflected on the others. – Damian

Hi stijn, I'm sorry this is a bit drawn out. The Results class itself does have a few more public methods but most of it will be private and since I wanted to avoid compile time dependency and also wanted to keep the implementation private I chose pimpl. ResultsImpl is where the reall party is at however, I have other classes which will need to use the ResultsImpl part of Results and I don't want to expose this in Results public interface. Hence my problem. Maybe i do have a design flaw in wanting Impls to cross-communicate.. – Nicknickel

this still means it's not really pimpl since the implementation is visiable to the outside. What you could do is something like this: write the getResults as getresults( std::vector<Result>& results, std::vector<std::shared_ptr<ResultImpl>> ). Then in getResults you make a bunch of sherd_ptrs and inject them into the Results, plus you push them into the vector. Other classes can then access the impl they need from the vector, while the Result itself also has a reference to the same impl through it's shared_ptr. It's still a bit messy though. – Fagin

Answer 2

Why exactly do your classes depend on details of each other? You may re-think your design. Classes should depend on abstractions.

If you really deem your design proper, nothing stops you from providing a "source-file-private" header, like:

include/
   foo.h
src/
   foo.impl.h
   foo.c
   bar.c

and then #include foo.impl.h in both foo.c and bar.c

foo.c:
    #include "foo.impl.h"
    ...

bar.c:
    #include "foo.impl.h"
    ...

But again: Generally,

Dependency Inversion Principle:

A. High Level Modules should not depend upon low level modules. Both should depend upon abstractions.

B. Abstractions should not depend upon details. Details should depend upon abstractions.

Also make very sure to check out SOLID and GRASP, especially the point about loose coupling.

Aromatize

are you suggesting that I have a ResultsImplInterface that I could then return in a public getter method on Results. Thus still keeping the actual implementation private?. – Nicknickel

If you hold it in a pointer or reference, just to forward it to another class later, then this would be applicable. – Aromatize

Actually, having the classes depend on interfaces won't solve my immediate problem. It won't fix me having to gain access to the Results classes pimpl member which is private. Making the impl object an interface isn't changing it's protection. – Nicknickel

Answer 3

Making the the data member _pimple public won't buy you anything as long as those other classes do not see the definition of its type ObjectImpl - which is the very thing Pimple set out to prevent.

What you can do instead is to add a private interface to your Object class which will allow befriended classes to do whatever they need to do with an Object.

Of course, the common disclaimers about friend being a tool that should be used as rarely as possible all apply.

Tropopause

Answer 4

It is unnecessary to qualify the friend thusly.

If you just use friend class OtherObject, then the OtherObject class may access the necessary internals.

Personally my Pimpl are just a struct (bundle of data) and I leave the methods to operate on it in the original class.

Damian