Re: [boost] [pimpl] Mini Review

Subject: Re: [boost] [pimpl] Mini Review
From: Artyom Beilis (artyomtnk_at_[hidden])
Date: 2011-05-27 07:37:34

> From: Vladimir Batov <vb.mail.247_at_[hidden]>
> 
> > Artyom Beilis <artyomtnk <at> yahoo.com> writes:
> > ...
> 
> Well, these  reviews are clearly "fun". Is it that I am getting old and touchy 
>or
> these  new kids on the block are getting vicious or they are learning C++  
>before
> they learn "please", "thank you" and "with all due  respect"?
> 
> Artyom, if you want to have a civilized conversation/discussion  or at the 
very
> least to maintain any *pretense* of a civilized  conversation/discussion, then
> you probably might like to have the other party  *willing* to talk to you. If 
>so,
> then *please* try to refrain from statements  like "without a knowledge of the
> problem domain", "you need", "you want" and  similar kind. I suspect quite a 
>few
> people might find such style as rude,  objectionable and arrogant. 
> 

Hello Vladimir,

I apologize from all my heart. I'm sorry if my tone was rude or disrespectful.

I want to do the discussion in positive way and I do not want to be 
disrespectful
to anybody. I'm sorry if it feels wrong.

However, it does not change the criticism to the library that I think can't
be accepted as it does not do the stuff required from Pimpl library.

I'm talking from point of view of somebody who actually uses and pushes
pimpl around and I think the library had took wrong design approach.

It is my opinion and I stand behind it.

> 
> ... I snipped the actual manually-crafted and  Pimpl-based code.
> 
> The difference is that the Pimpl-based version provides  the following
> functionality which your "flexible" alternative does  not.
> 

Ok, I'll answer each point separately, but in general, Pimpl based should
not expose implementation details. Pimpl class and normal class should
not look different from user point of view.

It is my opinion. It is correct design approach - decoupling an implementation
from the interface.  Exposing implementation details in Pimpl is bad design 
(IMHO)

> relational operators;

Two points:

1. pimpl object should be compared by value as the fact that there is a some 
   pointer behind is implementation detail so current approach  is incorrect
   from my point of view

2. The real relational operator should use implementation class and it does
   not provided automatically.

> safebool operator;

It is implementation detail, it should not be exposed to pimpl user.

> pimpl::null (and  pule-eee-ase do not tell me that *I* do not need that);

It is implementation detail, it should not be exposed to pimpl user.

> support for in-place  and replace boost::serialization;

Ok.. I need to take a look on it but in any case it requires
some action from user... Isn't it?

> support for value and pointer semantics  (and others if needed policies);

More policies required as I mentioned above.

> support for the Bridge pattern;

I don't see how it isn't supported with proposed clone_ptr.

> support  for Non-Virtual Interface Idiom;

No problem there for any pimpl implementation I'm aware of.

> the proper deployment of the incomplete-type  management technique (for the
> destructors to always work correctly).
> 

This is the only actual point I like in this library but I think
it should be solved in scope of some smart pimpl pointer and
not in the approach of this library.

> I  do agree that that's "not too much" as you put it. After all it's just  one
> tiny header file. However, you seem to like implementing (or rather  ignoring)
> that functionality for every pimpl you write.
> I seem to favor  finding a solution
> for a particular problem (even if that problem is as small  as a pimple ;-) ) 
>and
> then *re-use* that solution.

The problem that  additional parts should not be there for proper pimpl-class 
design
as they are implementation details.

> As I wrote in the original  article which you
> obviously did not bother reading -- "Writing a conventional  Pimpl-based class 
>is
> not hard. However, repeating the same scaffolding over  and over again is 
>tedious
> and error-prone. Why do that if we do not have  to?"
> 

First of all I did read the article and I do want pimpl in Boost but
not in the way this library suggests.

This is a list of problems I see in this library:

- Pimplized object should not derive from some other objects - wrong approach
  and design (IMHO) and exposes too much.

- No support of non-copyable implementations (with value_semantics)

- No support of custom implementation classes. Consider

     clss unicode_string {
     private:
         smart_ptr<icu::UnicodeString> pimpl_;
     };

- Better support of place holders (empty implementation object for future use)
  is required.

- No support of cloning underlying object (instead of copying)

- Thinks that are implementation details exposed to the interface,
  (noted above)

Best Regards,
and I apolagize once again if my review was rude.

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