Re: [boost] Review Request: Generalization of the Pimpl idiom

From: Vladimir.Batov_at_[hidden]
Date: 2008-04-02 20:45:49

...
> > That requirement to have
> >
> > Derived1::Derived1(implementation*)
> >
> > is unreasonable as it is clearly not related to to the Derived1 
*public*
> > interface. On the other hand reset()-based approach works fine and 
scales
> > fine.
> >
> > ...
> 
> I understand now. This will force to add an artifial *protected* 
constructor 
> to
> Derived1. Do you really think that this is a real problem for the 
developer 
> of Derived1?
> If I understand your library forbids this to the developper because 
> pimpl_base do not have the following protected constructor.
>     pimpl_base (implementation* impl) : impl_(impl) {}
> 
> I think that this will be a good adding to the pimpl_base  class.

Respectfully I have to disagree. The reason is that it has been long 
established precedent in C++ not to provide alternative 
mechanisms/tools/interfaces if the existing already does that. An 
immediate example OTOH is that an introduction of "superclass" keyword was 
rejected because a typedef would do the same thing. Granted, tediously and 
not as elegantly but still. Here IMHO it's the same story -- we are not 
talking about missing functionality but rather questionale interface 
sugar-coating which BTW requires the user to do more than he really have 
to ("force to add an artifial *protected* constructor").

...

> > I am not sure what that is supposed to demonstrate. You use 
infrastructure
> > to help you to do your job. If you decide that the infrastructuere 
does
> > not help you and discard that, you'll obviously have to re-implement
> > whatever functionality from that infrastructure you managed to deploy. 
If
> > one uses std::list and then for whatever reason goes ahead with his 
own
> > list implementation or merely swaps it for std::vector, he'll likely 
to
> > face the same issue. Trade-offs of the pimpl idiom are well-known -- 
> > another indirection and memory allocation.  In the end, one can deal 
with
> > memory-allocation by writing a custom allocator for
> > pimpl<Foo>::implementation.
> 
> If pimpl is here to hide the implementation, why it needs to extends the 

> interface of the initial class?
> Why the user of my class should suffer from the trade-offs of the pimpl 
> idiom at the inteface level?

Pimpl extend the interface so that the user does not have to provide those 
methods. Like comparison and validity check, etc. That does not come at 
the user's expence and, therefore, how exactly the user suffers from that 
escapes me.

> >> Do you really think that the pimpl idiom is helping me to hide my
> >> implementation. If I want to hide my implementation I need to 
restrict
> > to
> >> private inheritance.
> >
> > I cannot say about you but I do really think that the pimpl idiom is
> > helping me to hide the implementation. Private inheritance restricts
> > access but not hides the implementation.
> 
> I was talking about private inheritance from pimpl!!!

If you try deploying pimpl in applications, you'll quickly see that you'll 
need public inheritance. I do. Having said that, if you insist on 
inheriting privately, there is nothing stopping you. We are not discussing 
missing pimpl functionality or crooked implementation, right? 

> > struct Base { int k; };
> > class Derived : Base {}
> >
> > Here Base::k is not accessible in Derived but not hidden. More, if 
that
> > stuff is a commercial library, I can easily circumvent that 
restriction by
> > modifying Derived declaration. Pimpl really hides the implementation. 
See
> > for more about the idiom
> >
> > 1. Guru of the Week #24. http://www.gotw.ca/gotw/024.htm
> > 2. Herb Sutter. Exceptional C++ (Addison-Wesley, 1999)
> > 3. J. Carolan. Constructing bullet-proof classes. In Proceedings C++ 
at
> > Work'89 (SIGS Publications, 1989)
> > 4. James O. Coplien. Advanced C++ Programming Styles and Idioms
> > (Addison-Wesley, 1992)
> > 5. Eric Gamma et al. Design Patterns (Addison-Wesley,1995)
> 
> This is why I suggested to remove the null object and bool type 
conversion
> operators for pimpl<A>::value_semantics. Maybe it will be a good idea to
> give them protected acces.

I admit that "This is why" escpes me. Invalid objects are all over -- 
nullptr, shared_ptr(), -1, etc. Pimpl extends that concept onto actual 
classes and gives it uniform and proper implementation. I've been using 
this extensively and I do indeed need that... public. If one does not need 
that, he won't use that. That feature won't kick in by accident.

> >> Once we remove the public inheritance from pimpl, it is not eassy to
> >> implement all these extra functions and operators.
> >
> > Then do not remove public inheritance. I am far from being convinced 
that
> > insistence on private inheritance is justified.
> 
> Conveinced or not this is a way to mask the pimpl functions, and it 
seams to
> me the best way to use pimpl<A>::value_semantics. This is not the case 
with
> pimpl<A>::pointer_semantics from which it is better to inherit 
publically
> giving acces to the null object and the bool type conversion operators.
> This is only a comment on how I will use it :-)

Yes, that makes sense. Although if my usage experience is of any guidance, 
you'll swap private for public fairly soon. I guess, we'll see. 

...
> >> >> Could we inherit publicly from A and privatly from pimpl<B,
> >> >> A>::value_semantics?
> >> >
> >> > Cannot comment on this one as I am not convinced we need that (see
> > above).
> >> Imagine A3ppClass is a 3pp class ( we can not modify it), and before
> > using
> >> the
> >> your pimpl class we had:
> >>
> >> class MyClass : public A3ppClass {};
> >>
> >> How can we use pimpl without changing the interface of MyClass, 
neither
> >> adding nor removing any function?
> >>
> >> May be we can resume: do not use the pimpl library is you can not 
master
> > the
> >> complete inheritance hierarchy.
...
> In any case I don't see a real benefit of using pimpl when the base 
class is
> 3pp class and we can not change it.

Well, if you build a run-time polymorphic hierarchy on top of an existing 
3rd-party base class, then I tend to agree with you as you have to follow 
the design and strategy outlined by the base class designer. I do not feel 
like it is a pimpl inherent weakness. Havind said that, I'd shy away from 
coming to your generic conclusion based on that particular case. Unless I 
have to write deep run-time polymorphic hierarchies (like Widgets, etc.), 
I tend to cut inheritance short and I believe it is the strong current 
programming trend in C++. Therefore, in your case I'd try 

pimpl<MyClass>::implementation : public A3ppClass {}

then in MyClass I'd bridged to A3ppClass only the interface that MyClass 
needed:

MyClass::foo() 
{ 
        (*this)->A3ppClass::foo();
}

It might not be applicable in your particular case -- no one claims to 
have the silver bullet.

...

> >> class A : private pimpl<A>::value_semantics {...};
> >> class B : private pimpl<B>::value_semantics {...};
> >> class C : public A, public B {...};
> >>
> >> May be we can resume: do not use the pimpl library if you need 
multiple
> >> inheritance.
> >
> > I cannot see anything drastically wrong with the code above even 
though I
> > probably would not do it that way. So, I'll have to disagree with your
> > conclusion.
> 
> Anyway, we will do the same, don't use pimpl with multiple inheritance.

I would not say that I will not. If/when the time comes for me to deploy 
multiple inheritance, I cannot see myself hesitating because of the pimpl.

...

> > I have to disagee as I find that view over-simplified and in fact
> > incorrect. Pimpl or no pimpl in general no objects beyond 
value-semantics
> > PODs can be safely (or without additional effort) put into shared 
memory.
> 
> I hope so, and I expect that you will do what is needed to achiev this. 
I
> tried only to help you.

And it is much appreciated.

> Pointers are emulated in Boost.Interprocess with the smart pointer
> offet_prt.
> You can yet have inheritance without virtual functions.

Well, I would not insist on that without severe reservations, numerous ifs 
and buts.

> Please, could you take a look to the Boost.Interprocess, Boost.Intrusive 
and
> Boost.MultiIndex libraries.
> Even if not standards they are portable. :-)

Yep, had a quick look at boost.interprocess. Seems like it takes care of 
shared memory hasssle. Cool. I presume he managed it for UNIXes and 
Windows. Super cool.
And I see boost.multiindex supporting boost.interprocess allocators. Now I 
get it. Still I do not feel like rushing to support the feature. :-) We'll 
get the bridge when we get there. I'll keep it iin mingd though. Thanks 
for your patience and insistence. You see, I not THAT thick after all. :-)

> > Then, you'll have to provide custom allocators. The issue just
> > keeps more and more complicated. Do you actually have that setting or 
we
> > are talking hypothetically? If the former, then pimpl will not be your
> > primary concern. :-) If it is the latter, then I find it too ephemeral 
to
> > discuss effectively.
> 
> I insist. This is not hypothesis. I have to manage a lot of data shared
> between process, I use specific allocators, and synchronization 
mechanisms.
> I wanted only to know if I can hide the implementation of this classes 
with
> the pilm classes. That's all.

OK, I see you need that. :-) It'll take me sometime to get up to speed 
with boost.interprocess. Worse, for various reasons I cannot use 
un-official boost. So, go and figure.

> > If you do not have several processes accessing the same data and,
> > therefore, do not care for inter-process data-access synchronization,
> > inter-process vtbl pointer correction, etc., then I'd be wondering why 
you
> > use shared memory in the first place.
> 
> Who has said that I have no several process sharing the same data?

No, you certainly did not say you did not have several processes. Have you 
noticed there was 'if' at the beginning of my sentence? :-)

> But the data has no virtual functions. This is not a raison to don't 
want to
> hide its implementation.

Fair enough. You convinced me. Honestly, Unfortunately, or reasons beyond 
my influence I won't be able to extend pimpl support onto 
boost.interprocess in a short timeframe. Apologies.
..
> Thank you very much for your time and good luck for your review.
> I promise I'll try your library.

Thanks for your input. Much appreciated. 

Thanks,
Vladimir.