Re: [boost] pimpl library proposal

Subject: Re: [boost] pimpl library proposal
From: Krzysztof Czainski (1czajnik_at_[hidden])
Date: 2010-12-09 08:06:15

2010/12/9 Vladimir Batov <vbatov_at_[hidden]>

> Kris,
>
> Thanks for your input. It's much appreciated.
>
> Have you had a chance to read through the Pimpl documentation? Currently,
> the described Pimpl provides two policies -- pointer_semantics and
> value_semantics. In the pimpl.hpp code it looks as follows:
>

Yes, I have read that documentation ;-)

>
> template<class T>
> struct pimpl
> {
>   typedef pimpl_base<impl_ptr> value_semantics;
>   typedef pimpl_base<boost::shared_ptr> pointer_semantics;
> };
>
> More so, nothing is stopping us from extending it with, say, COW semantics:
>
> template<class T>
> struct pimpl
> {
>   typedef pimpl_base<impl_ptr> value_semantics;
>   typedef pimpl_base<boost::shared_ptr> pointer_semantics;
>   typedef pimpl_base<cow_ptr> cow_semantics;
> };
>
> Isn't it "customization of the copying policy"? Or did you mean something
> else?

I see. But if I understand correctly, for each smart pointer type, there
needs to be a specialization of pimpl_base written from scratch. Assuming
that a SmartPtr provides pointer operators and performs copying according to
some policy, You might consider writing a general pimpl_base<SmartPtr>.

Now suppose pimpl_base<SmartPtr> does accept any smart pointer: scoped_ptr,
shared_ptr, clone_ptr, cow_ptr, etc... This is where the creation policy is
needed. See below...

 And perhaps a way of customization of the creating policy, a factory
>> of some sort...
>>
>
> I am under impression that I do not quite understand what you mean by
> "customization of the creating policy" either. Isn't that quality achieved
> by overloaded constructors? If it is so, then the developer provides the set
> of overloaded constructors for his/her class. Regardsless of those
> constructors Pimpl does not need to be modified.

The default creation policy would be to create impl on the heap using
operator new, and passing the resulting pointer to SmartPtr. But for
example, when using shared_ptr, it might be a good idea to use a)
make_shared instead of operator new, b) a custom allocator/deleter.

Thus, I suggest making pimpl_base< SmartPtr, Factory = HeapFactory<SmartPtr>
> a general class with no specializations per SmartPtr.

 3. The user should have no way of telling if MyClass uses pimpl or not. It
> should be totally transparent.
>
I find it hard to visualize what you have in mind. Would it be possible to
> provide an example where you believe manually-written pimpl-idiom class is
> more transparent than a class derived from the proposed pimpl?

Well, I understand You expose the operator bool_type for example.

But that is not all. I believe a user is also someone deriving form MyClass.
Your pimpl takes part in runtime polymprphism, and for that it mus be
visible by derived classes.

> With Your pimpl, MyClass inherits from
>> pimpl_base, bringing in a lot of operations: operator*(), operator
>> bool_type() and others. They are protected, but they still are there -
>> potentially conflicting with operators, that MyClass might be providing
>> for
>> the user.
>> Therefore I'd prefere making pimpl a member over inheriting from
>> it.
>>
>
> Well, you omitted concrete examples/explanations clarifying why you prefer
> composition over inheritance in this particular case. Consequently, I have
> that nagging feeling that you are taking the "prefer composition over
> inheritance" a tiny bit too dogmatically. From where I stand I feel that
> Pimpl-derived class needs/wants to expose the complete interface of Pimpl
> (after all that's why those methods were introduced in Pimpl -- to re-use
> them). That indicates inheritance.
>
> In my view Liskov Substitution Principle is a perfect litmus test for
> "should I be inheriting from this type?". And in my view Pimpl fits the
> bill.
>

I just thought that, since the user should not know of pimpl's existence,
there should be no functions to inherit from it.

After all, functional inheritance (as opposed to architectural inheritance)
> is quite common programming technique.
>
> As for your concern about "a lot of operations" in Pimpl "potentially
> conflicting with operators" of YourClass, then my reading of the
> specification is somewhat different. Namely, as I understand, a method
> introduced in the derived class makes *all* the methods with the same name
> (not just the same signature) in the base class inaccessible.

True. So, what if the implementer adds operators * and -> to MyClass?
Accessing pimpl is still possible, but not as convenient any more.
satic_cast<implementation&>(*this)->... or an extra local reference is
always needed:

void MyClass::f()
{
  implementation& impl = *this;
  impl->...
}

> Namely, I do not see why a pointer_semantics-based Pimpl should not provide
> op==() automatically (as shared_ptr does). On the other hand, the user has
> to provide op==() for a value_semantics-based Pimpl.

For the pointer_semantics-based Pimpl: I do not think that MyClass should
have pointer semantics just because its pimpl has pointer semantics. So I am
against forwarding op== from the underlying shared_ptr.

For the value_semantics-based Pimpl: If the implementer wants op== in
MyClass, he should provide it. And impl should only contain data, and
perhaps helper functions, that would otherwise be private nonvirtual member
functions of MyClass without pimpl.

 Or it should not implement the possibility of a null object -
> boost::optional is for that.
>

Well, my view on the usefullness of pimpl::null() happens to be quite
> different. In fact, that pimpl::null is there because I use it a *lot*.
> Pimpl belongs to a smart-pointer-pattern family and shared_ptr and the likes
> provide such functionality (in a somewhat obscure form of shared_ptr() IMHO
> of cource).

On second thought, I agree, null() might come in handy, as long as there is
a way to disable it if it isn't needed.

Well, "pimpl should have nothing to do with polymorphism" is quite a
> determined statement. ;-) That mentioned functionality has its purpose -- it
> implements Non-Virtual Interface Idiom. If you have no need for that, that's
> OK. We should not be denying it to others, should we? In fact, I happen to
> use it to fight the complexities of architectural inheritance (where it's
> unavoidable).
>

I just think virtual functions should just stay in MyClass, and not go into
pimpl, because they should not be hidden. That is why I think "pimpl should
have nothing to do with polymorphism" ;-)

Thanks for the interesting discussion, Vladimir ;-)

For an example of how I use pimpl, and my pimpl helper, which I called
value_ptr, I shall quote my earlier post, adding an example of how some
member function of MyClass might be implemented in terms of pimpl:

MyClass.h:

class MyClass
{
public:
  MyClass(); // needed, but trivial
  ~MyClass(); // sometimes needed, but trivial
  void f();
private:
  struct Impl;
  value_ptr< scoped_ptr<Impl> > impl_;
};

MyClass.cpp:

struct MyClass::Impl
{
  /*...*/
  void f(int) {}
};

MyClass::MyClass()
{} // value_ptr automatically allocates an Impl instance

MyClass::~MyClass()
{} // value_ptr automatically deallocates Impl

void MyClass::f()
{
  if ( cond )
    impl_->f(1);
  else
    impl_->f(10);
}

The source code for value_ptr together with a few other related tools can be
found here:
https://libcz.svn.sourceforge.net/svnroot/libcz/trunk/boost

Regards, Kris