Boost logo

Boost :

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/


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk