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

From: vicente.botet (vicente.botet_at_[hidden])
Date: 2008-04-02 05:12:29

Hi Vladimir,

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

>> >>     Base::Base(int k) : base(new BaseImpl(k)){}
>> >>     Derived1::Derived1(int k, int l) : Base(new Derived1Impl(k, l))
>> >
>> > No, this initialization method is not allowed. One needs to use
> reset()
>> > instead as you show below. I considered the syntax above and rejected
> it
>> > on the grounds that it does not add new functionality (as reset()
> already
>> > does that) and it is not generic enough as it won't work for "Derived2
> :
>> > public Derived1"
>>
>> Why this syntax could not work? Could you elaborate more?
>
> For that syntax to work with a hierarchy every class needs to explicitly
> declare a constructor taking "implementation*". That is, in the following
> hierarchy
>
> Base : pimpl<Base> {}
> Derived1 : public Base {}
> Derived2 : public Derived {}
>
> pimpl, Base and Derived1 need to have those mentioned constructors so that
> the following could work
>
> Derived2::Derived2(int) : Derived1(new Derived2Implementation())
> {
> }
>
> 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.

>> > I cannot think of any particular implementation constraints
>>
>> Imagine that after using the your pimpl class which will do an extra
>> allocation, and due to performance I need to come back to a traditional
>> class. If my class inherits publicaly from pimpl I will need to provide
> the
>> same interface as when using the pimpl class.
>>
>> Before
>> class A : public pimpl<A>::value_semantic ...
>>
>> After
>> class A {
>>     public:
>>     // needs to implement publicaly the null function, bool_type
> operator,
>>     // and the ! operator.
>>     protected:
>>     // needs to implement all the protected part inherithed from
>> pimpl<A>::value_semantic
>>
>> }
>
> 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?

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

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

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

>> >> class A;
>> >>
>> >> class B : A, public pimpl<B>::value_semantics  {...}
>> >>
>> >> How the B implementation has access to A?
>> >
>> > I am not sure it is a good (from pimpl perspective) design. Pimpl is
> about
>> > separation of interface and implementation and implementation hiding.
> By
>> > inheriting from A we just threw all that out the window. So, doing
> that
>> > for B implementation does not seem to have much value. Having said
> that,
>> > if one indeed needs access to A inside B implementation, then keeping
> B*
>> > inside B implementation seems to solve the problem. With regard to
>> > handling polymorphic hierachies the Bridge description from GoF would
> be a
>> > good start. That'll be fully applicable to the proposed pimpl.
>>
>> Keeping a A* inside B implementation  will complicate a bit the B
>> implementation, isn't it?
>
> It depends. Even if it does, it'll be truly just a bit.
>
>> >> maybe we need something like that
>> >>
>> >> class B : public pimpl<B, A>::value_semantics {...}
>> >>
>> >> making the implementation of B inherit from A.
>> >
>> > "making the implementation of B inherit from A" is
>> >
>> > struct pimpl<B>::implementation : public A
>> > {
>> > }
>> >
>> > That certainly makes sense (to me anyway).
>>
>> How do you ensure this way that B  inherits from A?
>
> It feels like you want two separate hierarchies -- for interface and
> implementation. The application of the Pimpl idiom to polymorphic class
> hierarchies is well described in the "Bridge" section of GoF.

Sorry, but in this use case I can not change A.

>> >> 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.
>
> I am not sure how relevant it is to the technical side of things. We are
> not in a legal profession where for legal reasons they write "may contain
> traces of nuts" on a packet of nuts. I see infrastructure as providing a
> service. Decision about applicability of that service for a particular
> situation is application developer's responsibility. I dot thin the time
> of the infrastructure developer will be well spent trying to think of
> every situation where his code *cannot* be used.

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.

>> >> Can we manage with multiple inheritance?
>> >> class A : public pimpl<A>::value_semantics {...};
>> >> class B : public pimpl<B>::value_semantics {...};
>> >> class C : public A, public B {...};
>> >
>> > Sure... with all the relevant multiple-inheritance perks/drawbacks.
>>
>> Imagine that I have up to now
>> class A;
>> class B;
>> class C : public A, public B {};
>>
>> and that I have no problems with multiple-inheritance because I have no
>> common class on the inheritance hierarchy (even if I don't like it very
>> much).
>> Now I'd like to use your pimpl classes and now a found a common class,
> or
>> atleast some common functions or operators. Does it means that again I
> need
>> to
>> use private inheritance.
>
> Yes, I understand what you describe and 'no' I do not think it'll be an
> issue. And, no, again, in my view private inheritance does not help you
> one bit. (Just admit you like "her", don't you. :-) )
>
>>
>> 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.

>> >> How pimpl interacts with STL containers?
>> >>
>> >> stl::list<A> al;
>> >> B b;
>> >> al.push_back(b);
>> >> // ...somewhere else
>> >> A a =al.pop_back();
>> >>
>> >> How 'a' should behaves? like a A or like a B?
>> >
>> > I presume
>> >
>> > struct A : pimpl<A>::... {}
>> > struct B : public A {}
>> >
>> > Then the result depends if A is of value_semantics or
> pointer_semantics.
>> > With pointer_semantics it behaves as, say, boost::shared_ptr would
> behave.
>> > Otherwise, during copying B part will be chopped off. So, "A a" will
>> > behave as A.
>>
>> OK, I have missed that. Does it means that only pimpl<A>::value_semantic
>> is sustitable for a class which was already used with STL containers?
>
> No, it does not. I am overwhelmingly using pimpl::pointer_semantics and
> those are happily in various containers. And those objects behave
> polymorphically because they have pointer semantics. If it is not the
> desired behavior, then value_semantics should be used instead. It is up to
> the class designer to decide.

OK, it is clear now.

>> >> The STL container user that
>> >> shouldn't knowns nothing about A implementation, expect that it will
>> > behaves
>> >> as a A, but it think that 'a' will behaves like a B. How can we
> protect
>> > from
>> >> this situation?
>> >
>> > I probably do not understand something as I still do not see what it
> is to
>> > protect from. A pimpl-based class behaves as any ordinary class (as it
> is
>> > in fact an ordinary class).
>> >
>>
>> May be we can resume: do not use the pimpl pointer semantics if your
> class
>> has value semantics.
>
> Well, I probably can agree with this conclusion... although I am temped to
> argue that in that case we'll have to put similar "warnings" everywhere.
> Like "do not use knife when you actually need a spoon", etc.

Sorry for the tautology

>>
>> In order to understand better your purpose:
>> * Could you give an example in which the object has value semantics and
> the
>> implementation pointer semantics?
>> * Could you give an example in which the object has pointer semantics
> and
>> the implementation value semantics?
>
> Private implementation does not have any particula semantics. It is the
> interface (pimpl-derived) class who has pointer_ or value_semantics...
> meaning how it treats the implementation -- aloows to share it or copies
> it.
>
>>
>> >> Could we store a non polymorph pimpl object in shared memory?
>> >> Could we store non polymorph pimpl objects in a container in shared
>> > memory?
>> >
>> > I am not convinced these special cases require anything from the
> pimpl.
>> > Although these certainly need more effort from the developer. Issues
> and
>> > means to deal with those seem to be the same as if one tried to store
> a
>> > pointer or std::auto_ptr or a boost::shared_ptr in shared memory.
> Given
>> > it's all in the developer's hands (implementation can be allocated
>> > explicitly and then associated via reset()) it'll be the developer's
> pain.
>> > This seems to be merely part of overall painful experience of dealing
> with
>> > shared memory. :-)
>>
>> You are right, auto_ptr and shared_ptr do not work on shared_memory. The
>
>> difference is that as you state a pimpl is not a pointer, and so we
> should
>> not suffer from the pointer liabilities. When a user use std::auto_ptr
> or
>> boost::shared_ptr he knows that there are some pointers not too far. In
> the
>> case of the pimpl the user has nothing to be with the pimpl classes and
> its
>> pointers.
>
> 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.

>> Imagine that  you are using a class A in shared memory and even a
> container
>> of A in shared memory and you want now to use your pimpl class to hide
> the
>> implementation of A. What I can do?
>
> Shared memory is a delicate area. It's like embedded programming where not
> all language resources are readily available. If your A class can live in
> shared memory, then it is essentially a POD. Say, consisting of a few
> integers, no pointers, no inheritance.

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

Otherwise, two processes cannot
> access/manipulate that same object in shared memory (not easily anyway).
> Still, those processes must have access synchronization issues resolved.
> More so, you mention a container in shared memory. Is it a standard
> container?

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

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

> 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?
But the data has no virtual functions. This is not a raison to don't want to
hide its implementation.

>> Do not forget that you have two pointer member?
>>
>> template<class Interface>
>> template<class Implementation>
>> class pimpl<Interface>::impl_ptr
>> {
>> ...
>>     traits const* traits_;
>>     Implementation*    p_;
>> };
>>
>> May be you need to add some policies to your class in order to manage
> with
>> this (see for example void_pointer in Boost.Intrusive)?
>>
>> May be we can resume: pimpl library can not be used for classes that can
> be
>> stored in shared memory (atleast in his current state).
>
> Well, again I am not sure we should rush with conclusions based purely on
> hypothetical assumptions without trying. I admit I did not try using pimpl
> with shared memory as I do not use shared memory these days. It's almost
> not portable, limiting, hard to handle, and most (all?) of the time is
> better implemented as a multi-threaded app anyway.

Without trying I know that the way you use the pointers do not allow to put
your class is shared memory.
You can see offset_ptr in Boost.Interprocess.

>>
>> >> I hope that you will find good solutions to these problems.
>> >
>> > So, do you find my answers to your satisfaction?
>> >
>>
>> Not completly, but I hope that this discusion will serv to construct a
>> better
>> pimpl library or atleast to show the limits where this library is not
> really
>> adequated.
>
> Well, I certainly biased :-) but I am trying to keep an open mind and so
> far I do not see anything that I'd consider as a limitation. Call me a
> stubborn nut if you like.

Doesn't matter, I understand your concern.

>> The question is, can we subtitute a class A by a class A : public
>> pimpl<A>::xxx_semantics? I think that it will be better to restrict the
> null
>> object and the bool type operator to the pointer semantics.
>>
>> What do you think?
>
> I consider value- or pointer-semantics to be the behavior when null() is
> clearly a functionality. They are very much orthogonal to each other.
> Foo::null() clearly declares an invalid (unusable) object regardles of its
> semantics. I see value in it. If one does not need that functionality, he
> probably won't use it. As simple as that. I did not hear compellling
> arguments *not" to provide that functionality for value-semantics classes.

It was only an idea :-)

> Thanks,
> Vladimir.

Thank you very much for your time and good luck for your review.
I promise I'll try your library.
_____________________
Vicente Juan Botet Escriba