Re: [boost] [pimpl] Mini Review

Subject: Re: [boost] [pimpl] Mini Review
From: Artyom Beilis (artyomtnk_at_[hidden])
Date: 2011-05-26 08:47:17

> From: "Stewart, Robert" <Robert.Stewart_at_[hidden]>

> Thomas Klimpel wrote:
> > Artyom Beilis wrote:
> > > Should The  library be accepted in Boost:
> > >  ========================================
> > >
> > > No it should  not. It is "a fancy" library that was written
> > > without a knowledge  of the problem domain. It tries
> > > to solve problem in smart way  making it overcomplicated.
> > >
> > > IMHO old std::auto_ptr is  much more suitable
> > > for writing pimpl-objects then  Boost.Pimple.
> > >
> > > For easy pimpl-ideom you do not need a  fancy class
> > > but rather a set of pimpl-suitable smart  pointers.
> >
> > I haven't reviewed the Pimpl library, but this  comparison to
> > "old std::auto_ptr" looks very strange to me. If I  understood
> > correctly, one of the reasons why "old std::auto_ptr"  is
> > deprecated in favor of "std::unique_ptr" is that "old
> >  std::auto_ptr" will silently break the default generated copy-
> >  constructor and assignment operator of a class containing an
> > "old  std::auto_ptr" as member. "std::unique_ptr" on the other
> > hand will  disable default generated copy-constructor and
> > assignment operator and  only default generate the "move"
> > versions of these, so the user will at  least realize that he
> > has to manually provide correct versions of  copy-constructor
> > and assignment operator.
> 
> It is these issues and  the several manual steps required to implement the 
>Pimpl
> Idiom that this library  tries to address and simplify.
>

Simplification does not imply that user must derive its own class from
other class?!

It does not simplifies things, it makes them more complicated.

Consider following copy_ptr that both safe and simple.

   template<typename T>
   class copy_ptr {
      template<typename O>
      explicit copy_ptr(O *p) : ptr_(p), deleter_(deleter<O>),copy_(copy<O>) {}
      copy_ptr(copy_ptr const &other) :
          ptr_(0),
          deleter_(other.deleter_);
          copy_(other.copy_)
      {
         ptr_ = copy_(other.ptr_);
      }
      ~copy_ptr() {  if(ptr_) deleter_(ptr_) }
      ...
   private:
      T *ptr_;
      void (*deleter_)(T *p);
      T *(*copy_)(T const *p);
      template<typename O>
      static void deleter(T *p) { delete static_cast<O *>(p); }
      template<typename O>
      static T *copy(T *p) { return new O(static_cast<O const &>(*p); }
   }

It is safe for use by total newbie, but it does not require
from a user to derive its own class from other one.

The proposed solution is too complicated and overkill.
It does not makes things simpler.

> It is totally clear that one can use  shared_ptr, unique_ptr, etc.
> or even a raw pointer to implement the Pimpl  Idiom.  
>
> However, each such choice requires different member functions to  effect proper 
>copy and equality semantics,

With copy_ptr above no problem with copy semantics or other semantics.

Also note Boost.Pimpl's equality semantics is wrong.

You don't compare pointers to implementations but implementations themselves.

So 

   operator==(Book const &other) ( impl_ == other.impl_ /* as pointer */ }

Is wrong, you need to compare *impl_ == *other.impl_

And it is not possible to do in header file as actual implementation required.

> It may be that Artyom has used Pimpl so often that he never  makes mistakes
> in its implementation for each use case.  He may understand  the issues
> completely and may always do the right thing.  That doesn't mean  that
> others will do as well.  Libraries exist to capture design and
> implementation details so others can build upon that effort,
> even without fully  understanding the issues.
>

That is why I had created a set of smart pointers for pimpl.

It has nothing to do with making mistakes. That is why copy_ptr solves the 
problem
and hold_ptr (that is just non-copyable copy_ptr) solves the problem as well.

They are safe but also simple.
 
Simplicity is the key. Derivation from other class is not simplicity.

Writing 

  template<>
  class pimpl<foo>::implementation

Over

  foo::implementation

Is not simplicity.

Especially when it limits your to very specific type when you may hide some
other existing complex type that you may just not show.

> It may be that this library needs to be  extended to support
> cloning, holding, and other use cases the author hadn't  identified.  
>
> That hardly justifies saying that the author does not have  "knowledge of the 
>problem domain."

I'm sorry if it seems rude. However, some design issues in the
library design make me feel that too many important uses cases
are just not covered, that's it.

For example, consider something like

  xml_document {
  public:
    ...
  private:
    smart_ptr<xmlDocPtr> pimpl_;
  }

Very popular type use case when one class if a facade
of other complicated one or one that is does not exposed
to user.

Can't be done with Boost.Pimpl as pointer to implementation
is restricted to specific type.

The problem is that the library solves only one particular
use case and does not see too many other very popular
use cases existing in the real software.

 Artyom Beilis
--------------
CppCMS - C++ Web Framework:   http://cppcms.sf.net/
CppDB - C++ SQL Connectivity: http://cppcms.sf.net/sql/cppdb/