Re: [boost] [pimpl] Mini Review

Subject: Re: [boost] [pimpl] Mini Review
From: Krzysztof Czainski (1czajnik_at_[hidden])
Date: 2011-05-26 06:46:12

2011/5/26 Artyom Beilis <artyomtnk_at_[hidden]>

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

Hello,

I totally agree with your review, Artym, except that I don't have knowledge
about ABI changes to agree or disagree.

I had a discussion with Ivan on this list about Boost.Pimpl some time ego,
when he was probing interest, and my conclusion was similar to yours, but I
had used different arguments. I won't bring my arguments, because I find
your argumentation better and sufficient ;-)

Anyway, I think boost lacks tools you called copy/clone_ptr, and hold_ptr. I
wrote my own versions of those, and use them widely for Pimpl just like you
say in your review. Would it make sense to propose them as an addition to
smart_ptr maybe? I know there were some clone_ptr propositions in the past,
so I never proposed my versions... But maybe in the context of Pimpl, they
would be found useful?

Regards
Kris