[boost] [pimpl] Mini Review

Subject: [boost] [pimpl] Mini Review
From: Artyom Beilis (artyomtnk_at_[hidden])
Date: 2011-05-26 05:22:33

Hello All,

Because a pre-review runs this days in collaborator tool I still want
to provide some feedback on this library I was looking on long
time ago.

    - What is your evaluation of the design?

---------------------------------------------
This is the major problem of the library:
It tries to do simple things overcomplicated.
---------------------------------------------

Take as an example:

  struct Book : public pimpl<Book>::value_semantics { ... };

Is an attempt to do things in "fancy" way where it is not needed.

  struct Book {
  public:
  private:
     struct implementation;
     smart_ptr<implementation> pimpl_;
  };

Is much clearer.

The proposed solution is overcomplicated hard to read and in general makes
more troubles then helps, makes the code less readable by average programmers.

------------------------------------
shared_ptr as pointer model is Wrong
------------------------------------

You almost never want to have shared object semantics
for pimplized object.

I use pimpl ideom widely in my projects CppCMS, CppDB and even in Boost.Locale
and I used shared semantics only once where it was explicitly shared.

In rest of the cases I had found myself using following smart pointers:

1. copy_ptr -  pointer the copies underlying object on its copy.
   i.e. something like

   copy_ptr(copy_ptr const &other) : p_(new Object(*other)) {}

   Copy the object on copy.

2. clone_ptr - pointer that clones underlying object on its copy. 
   i.e. something like

   copy_ptr(copy_ptr const &other) : p_(other->clone()) {}

3. hold_ptr - pointer with same const-semantics as an object it points to

   T const &operator*() const;
   T &operator*() ;
   T const *operator->() const;
   T *operator->() ;

They cover almost all use cases, so with addon of shared_ptr
it covers ALL use cases.

In any case current boost::scoped_ptr or std::auto_ptr is much
more suitable smart pointer for implementing pimpl then
Boost.pimpl.

The only advantage I can see in boost.pimpl over scoped_ptr or auto_ptr
is that it does not require explicit destructor.

i.e.

   class Foo : boost::noncopyable {
   public:
         Foo();
         int x() const;
         void x(int v);
   private:
         struct data;
         std::auto_ptr<data> d;
   };

No good. You need to add

  ~Foo();

So it would know to destroy Foo::Data correctly.

But this can be easily solved on hold_ptr, copy_ptr and clone_ptr level
same as it solved for shared_ptr and in reality even this not needed
as it is better to add empty destructor in cpp.

Spare Pointer use case
------------------------

Another important use case in the case when pimpl is used to handle ABI 
stability and
in general the object has spare "pimpl" or d-pointer for future uses.

Consider:

  struct book {
  public:
     book();
     book(book const &);
     ~book(book const &);
     book const &operator=(book const &);
     std::string title() const;
     std::string author() const;
     void title(std::string const &);
     void author(std::string const &);
  private:
        std::string author_;
        std::string title_;
     struct data;
     copy_ptr<data> d;
  };

Now in book.cpp we define data as 
struct book::data {};

And we do not initialize d at all. It is fast and efficient 
object that does not use data at all.

Now when we decide to add new member "isbn" without breaking ABI.

in book.cpp we define in book.cpp

   struct book::data { std::string isbn_; }

And now we initialized the d pointer in constructor.

And add new members in book.h

   std::string isbn() const
   void isbn(std::String const &);

And implement them

   std::string book::isbn() const { return d->isbn_; }
   void isbn(std::string const &s) { d->isbn_ = s; }

This is very important use case the the author does
not even relate to.

    - What is your evaluation of the implementation?

Generally not looked into too deeply but it is does
not follow boost standards:

- Naming conventions
- Documentation
- Build

So it is not even ready for the review (even thou
it can be easily changed as it is a small library)

    - What is your evaluation of the documentation?

1. The documentation should be in HTML file not on
   some Internet web site.
2. Rationale part missing

The documentation in not Boost-Ready

    - What is your evaluation of the potential usefulness of the
      library?

Useless for 90% of pimpl use cases.

    - Did you try to use the library?  With what compiler?  Did you
      have any problems?

No I hadn't

    - How much effort did you put into your evaluation? A glance? A
      quick reading? In-depth study?

About 4-5 hours of studying 

    - Are you knowledgeable about the problem domain? 

Yes, I work with pimpl-ized object on daily bases in many
different approaches including implementing my own
smart pointers for pimpl (clone_ptr, hold_ptr, copy_ptr)

- ABI Stability
- Compilation time reduction,
- Decoupling of the interface from implementation details.

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.

Best Regards,

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