Re: [boost] [pimpl] Mini Review

Subject: Re: [boost] [pimpl] Mini Review
From: Ivan Le Lann (ivan.lelann_at_[hidden])
Date: 2011-05-26 06:15:55

----- "Artyom Beilis" <artyomtnk_at_[hidden]> a écrit :

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

Your code would rather simulate pimpl<>::pointer_semantics.
And you do not mention the boilerplate code generated.
 
> 
> 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
> ------------------------------------
> 
(snip lots of smart_ptr related lines)

Unless you want pointer_semancics ...
As for value_semantics, shared_ptr is not used.

> 2. clone_ptr - pointer that clones underlying object on its copy.
>   i.e. something like
>
>   copy_ptr(copy_ptr const &other) : p_(other->clone()) {}

Precisely what seems to be used in the library for value_semantics.

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

An important advantage as many developpers get trapped here.
And it is "only" a warning to do so ...

> 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 with Pimpl, you would put author_ and title_ in book::data.
I failed to see the point here. You seem to say that Pimpl cannot 
be used for ABI stability, which seems wrong to me.

Regards,
Ivan.