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

From: vicente.botet (vicente.botet_at_[hidden])
Date: 2008-04-03 07:55:15

Hi Vladimir,

From: <Vladimir.Batov_at_[hidden]>
Subject: Re: [boost] Review Request: Generalization of the Pimpl idiom

> ...
>> > 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 don't want to force any developer to do like that, I wish only the
pimpl library allows him to do in this way if he considers that it is a
better choice. I don't really like to have this implementation pointer
initialized to null and been reinitialized on the same constructor with the
correct implementation. Note that you have already do a good work allowing
to forward construct for the class inheriting directly from the pimpl (first
level).

By the way, do you plan to parameterize the number of arguments allowed,
using maybe preprocesor metaprograming? Yes I know that this is quite ugly,
but do not limit the user of the library.

>> > 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.

The user will do as he likes. In my case I will surely use private
inheritance for value_semantics in order to hide this addins.

>> >> 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?

You are right, there is nothing missing. I can achiev my goal using private
inheritance. Atleast I  hope so.

>
>> > 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.

It's OK. Some users could find this operations usefull.

>> >> 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.

I will let you know if this is the case. For sure.

>> >> >> 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.
>
Neither me.

>> >> 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.

Could you add into the documentation a complete and concrete example of
multiple inheritance with pimpl?
This will help the user to understand how eassy this task could be?

>> > 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. :-)

I understand now why you were reticent to use shared memory.

>> > 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.

Do you mean that you can use only official boost. The release 1.35 delivered
last week contains already Interprocess and Intrusive (see void_pointer)
libraries.

>> > 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.

Apologies accepted. Let me know if I can help you with the shared memory
issues.

>> 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.

You are welcome.

Best regards

_____________________
Vicente Juan Botet Escriba