Re: [boost] [pimpl] Mini Review

Subject: Re: [boost] [pimpl] Mini Review
From: Vladimir Batov (vb.mail.247_at_[hidden])
Date: 2011-05-28 02:03:53

> "Artyom Beilis" wrote
> ...
> 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.

Artyom,

I honestly believe that offending anybody is the last thing on your mind and
your primary/only objective is of technical nature. However, you'll probably
agree that accusing someone you-know-nothing-about of being "without knowledge
of the problem domain" oversteps that technical line and looks more like a
unwarranted personal attack.  What's worse is that after such a personal
"treatment" the person you are trying to communicate with gets understandably
defensive and is not open to the conversation. So, your otherwise useful other
suggestions/comments are not being heard. 

>From your CV I read you hit 30yo this year. Wonderful age. It feels like the
world is your oyster. At 30 one still has not lost the energy and enthusiasm
(and illusions) of 20yo and at the same has got the knowledge and the means to
have things done. It feels like the world and its forces are well understood;
and you know the *right* way; you know the destination; you know how to get
there. So, anyone doing things differently is an illiterate moron "without
knowledge of the problem domain".

I am unfortunately considerably older (and that sucks let me tell you). I had
more time to learn and as a wise saying goes – the more I learn the more I
realize how little I know. So, with time I might ultimately agree with you that
I am an illiterate moron "without knowledge of the problem domain"… but I am not
there yet. So, let’s talk Pimpl that you know so well and so eager to teach us
all how to use it properly (a little tease won’t hurt, will it?). ;-)

> … Pimpl based should not expose implementation details. 

Are you saying “my” Pimpl exposes the implementation details? Have you had a
chance to read the original article? “My” Pimpl does expose the implementation
details… but only to the implementer of those “details”, not to the user.

> Pimpl class and normal class should not look different from user point of view.

I probably do not understand what you are trying to say. As I read/interpret it,
it’s plain wrong. Pimpl tries to solve exactly what is wrong with the “normal”
class – tight coupling of interface and implementation. So, Pimpl and “normal”
classes cannot possibly look the same. If by “the same” you mean the interface,
then yes, in that respect Pimpl is no different from “normal” classes. In fact,
it *is* a normal class.

> It is correct design approach - decoupling an implementation
> from the interface.  Exposing implementation details in Pimpl is bad design 

Exposing implementation details in Pimpl is not just bad design... it’s just not
the Pimpl idiom to begin with. I feel you might like to consider having another
look at “my dear” Pimpl and reading the original article to realize that
separation of the implementation and the interface are as dear to my heart as it
is to you.

> 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

Well, what ceremonies between friends, right? So, I’ll give it to you straight,
my friend. That’s rubbish. The correct answer is “it depends”. Indeed, as I read
from your other Pimpl-related posts, your usage patterns are
overwhelmingly/exclusively with pimpls with value semantics. My pimpl usage
patterns are different (can you imagine? ;-) ). They happen to be overwhelmingly
with pointer semantics. I’ll give you a simplified example (so do not jump in
with “you all are doing it all wrong!”). We read database records and we cache
them. We cache them as shared_ptr-based Pimpls and we pass them around. So,
everyone using a particular record happens to share the same underlying record
data. If that record changes in the database, we discard the old version, cache
the new version, we send out update notifications. So, if one happens to keep a
Pimpl to that record, then he will have the original (now old, which he kept)
and the new record. We deploy that mechanism in several projects doing railway
traffic monitoring (of the whole city), airline scheduling and rostering, etc.
So, it’s not exactly chicken feed. Your projects are probably of different
nature and you probably do not need features I need. That’s why “my” Pimpl
provided pointer-semantics to keep me happy and value-semantics to keep you
happy… although for unknown reasons you are adamant in not noticing that “my”
Pimpl offers the functionality you are after. 

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

If I was talking to a friend (after two glasses of vodka), I’d probably say that
such a blunt statement is naive and immature. Given I prefer beer and we are
only getting to know each other ;-), I’ll stop short saying I am not sure I can
agree as I am under impression that the correct answer is “it depends”.

For value-semantics Pimpls you are probably correct for the majority of cases.
However, there is a bigger world out there which does not play by or follow the
rules you set in your nick of the woods.

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

I beg your pardon?! Since when safebool and pimpl::null have become
“implementation detail” and “should not be exposed to pimpl user”. The Pimpl
idiom allows the user to have these perks that raw pointers provide (like no
data behind the pointer or NULL pointer). Denying that to the user is nothing
short of robbery. Even many value classes provide that functionality. Like many
classes construct an invalid object with a default constructor. Still, if you
want to deny it to your users, use private inheritance. Just do not deny it to
the whole world.

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

Well, anything “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.

“My” Pimpl provides two most deployed policies. I had no substantial feedback
about other policies needed. Is it a good reason enough to say that the Pimpl as
a whole should be rejected? Maybe “accepted with new policies written as the
need arises?” ;-)

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

Have you actually tried deploying the above-mentioned patterns with the example
you provided along side the Pimpl-based example? If you are using some other
smart pointers instead, then I won’t be surprised they provide functionality
*similar* to Pimpl to support deployment of those patterns. It does not make
“my” Pimpl bad. It only means that I submitted something that you have an
alternative implementation 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.

Aha, you insist on the design like 

struct my_class
{
    interface;
    private:
    pimpl_ptr implementation_;
};

I do not see it much different from the *tactical* inheritance like

struct my_class : pimpl_ptr
{
    interface;
};

Which I find cleaner from user perspectives and often more powerful. So, if do
not like safebool and relational operators – use private inheritance. Just do
not deny that functionality to others and do not tell them that they do not
need/want that. 

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

Our interpretations of “proper pimpl” obviously differ. Again, if you insist on
denying some features to the user, use private inheritance; do not deny that
functionality to others. 

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

I honestly want to believe you. It’s just that I felt you insistently
misrepresented “my dear” Pimpl as solely shared_ptr-based Pimpl (which is not
good for your use-cases) and demanded copy_ptr-based Pimpl mysteriously failing
to notice value-semantics Pimpl functionality staring at you.

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

Here we go again. :-) Look, Artyom, I cannot even be angry with you anymore. You
are like a child, really – you stomp your foot and say – wrong design. End of
story. Just accept it – if you wear shoes size 44, then it does not make any
other sizes wrong. Other people *need* different sizes and styles and colors. 

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

That’s incorrect. If one does not implement copy constructor for the
value-semantics Pimpl, then it’s not copyable. More so, kick in
boost::noncopyable at any time if you need it.

> - No support of custom implementation classes. Consider
> 
>      clss unicode_string {
>      private:
>          smart_ptr<icu::UnicodeString> pimpl_;
>      };

I do not understand what you mean by “custom implementation classes”. All
classes derived from Pimpl are “custom implementation classes”. They are just
not implemented as you like it.

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

That “design” is clearly misguided. Borrowing from one of my Internet friends
it’s “wrong approach and design (IMHO)”. Pimpl supports modifications naturally
by separating interface from implementation. So, as long as interface stays, the
implementation can grow/change as you please. No need for those “place holders”.

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

I am not sure if you noticed by now but “my dear” Pimpl is policy-based. If
there is enough demand for that cloning, we’ll provide 

pimpl
{
    typedef class pimpl_base<cloning_policy> cloning_semantics;
}

> - Thinks that are implementation details exposed to the interface,

Wrong again. Private inheritance shuts out *all* pimpl API from the user. So,
the implementer is free to pick and choose. So, “my dear” Pimpl gives people a
choice and that’s what makes it a *library*.

V.