Re: [boost] [pimpl] Mini Review

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

> Artyom Beilis writes:
> ...
> I'm sorry but I have never called anybody "an illiterate moron", 

That was my "artistic interpretation" of the words you were quite happy to use.

BOOST_ALMOST_ASSERT(
    "an illiterate moron" == 
    "without knowledge of the problem domain");

> even
> thou I fully stand behind the fact that current Pimpl library covers
> very partial set of use cases. That is why feel it does not fit the 
> problem domain.
> 
> This is my opinion, it is different from yours but this is what reviews
> are for - provide an opinion that may differ from the opinion of the 
> library author.

Indeed, this is a forum where everyone is entitled to his opinion. However,
things that you so unfortunately sprinkle over your posts sound more than just
your opinion. More like a patronizing lecture or a scolding of a naughty child.
Are you sure you have the credentials to tell us all what we (I quote) "should
not care about" or "almost never want". Are you sure you have the right to speak
for the "average programmers", to decide what is "overcomplicated", "hard to
read" and "makes more troubles" for them? Are you sure you are well-qualified to
say what is "useless" and what "90% of pimpl use cases" are?

... And thanks, man, for not calling me a moron "EVEN thou" you stand behind
your opinion. :-) 

> ...
> I'm not trying to teach anybody but I have an obligation as a reviewer
> to bring up all the problems I see with this library.

To begin with you do not "bring up all the problems". You bring up the
details/issues that you *perceive* to be potential problems. Your perception (as
everyone else's mine obviously included) might turn out to be spot-on or wrong
(can you imagine?) for one reason or another.

Secondly, it's not a review. There is no Review Manager, there is no time frame,
there is no outcome/resolution. Rob Stewart needed to test that new Collaborator
tool. So, he thinly-veiled that need into a "pre-review" offer and lured me into
that trap for seemingly no particular gain for me.

>>> … 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.
> 
> Only if you inherit from pimpl<Foo>::xyz_semantics privately but then... You
> do not get for example operator== that you may actually want in case of 
> pointer_semantics
> but not want to expose where the underling object is null or not. 

You have to make up your mind. Isn't it that "shared_ptr as pointer model is
Wrong. You almost never want to have shared object semantics for pimplized
object"? Does your alternative implementation do it all better and provides
op==() but not the "implementation detail" which I am not sure what you are
referring to? "My dear" Pimpl gives us *all* a choice -- I can use
shared_ptr-based deployment with all the perks I value and you can use
value_ptr-based private-inheritance deployment and have full control what you
expose to the user. 

>> My pimpl usage patterns are different. They happen to be overwhelmingly
>> with pointer semantics.
> 
> I'm not telling that this is a valid use case. Of course it is, but
> it is only one of them.

Again, the majority of other use-cases that are important for your deployment
are covered by the value-semantics Pimpl (or can be covered by new policy
classes). Some of the use-cases that you describe as vital for Pimpl deployment
I strongly disagree with. Like

struct
{
    string title_;
    string author_;
    struct impl;
    smart_ptr<impl> impl_;
}

Such a half-Pimpl half-not monster is not a Pimpl and feels wrong on so many
levels. Consider yourself lucky you are not working on our projects and do not
have to pass our code review. :-) 

> You can successfully deploy this use case with using shared_ptr as member
> that represents opaque pointer.

Here we go again. I am sure you know better what I can and what I cannot... even
though you do not deploy shared_ptr pimpls. If I could, then I would. After
doing it over and over again I just noticed repeating the same stuff... which I
packaged up in "my dear" Pimpl to avoid repeating it. Is it a lot? Nop. Is it
useful to deal with a thought-through idiom implementation rather that repeating
that by hand? I think so. You might disagree.

>>> 2. The real relational operator should  use implementation class and it does
>>> not provided  automatically.
>> 
>> agree as I am under impression that the correct  answer is “it depends”.
> 
> Yes, it depends, but this is what your library suggests by default.

Wrong again. "My dear" Pimpl *delegates* that functionality to the underlying
policies. Therefore, those relational operators are provided for
shared_ptr-based deployment as it makes sense and shared_ptr provides those. For
value_ptr-based implementation there are no default relational ops.  

> But as I noticed before private inheritance would remove some other things
> that in some cases you may use.

Indeed, I need/use the functionality Pimpl provides. I do not privately inherit.
You insist that that's too much exposure. So, I say, inherit privately if you
like, and do the rest by hand. And I am sure you know how to selectively enable
otherwise disabled base-class functionality:

class derived : class base
{
    void foo() { base::foo(); }
} 

> ...
> Library does not even provide an option to have a pimpl as member
> that is why I think it is inflexible.

Clearly "my dear" Pimpl does not do it *all* for you. It does not *even* provide
that option above. ;-) There is something that you still have to do by yourself
or you deploy plethora of other smart pointers. 

The library addresses a certain well-defined concept/issue. It is about
constructing a Pimpl class easily, safely and painlessly. How you deploy that
Pimpl later is up to you. If you want to make it a member of another class, be
my guest. You might have some very special/specific use-cases. Those use-cases
are not necessarily to be covered by Pimpl. Maybe some other smart pointer is in
order. Horses for courses. 

> I does not do what **I** personally expect from the library and many
> others.

Of course, it does what you want. Value_prt-based, private inheritance. It just
happens to do it somewhat differently. Something you refuse to
admit/accept/acknowledge. ... And ***I*** like the "many others" part. :-)

> For the record I had once a discussion about pimpl with somebody
> that had same opinion - it is over-complicates simple things
> that should not be this way.

Well, I do not know how to reply to this "argument". That would probably
convince me ;-) ... but then I remembered that quite a few people in *my*
vicinity are happy to use it and do not find it "over-complicated". Then I
remember that after the original article was published Herb Sutter was kind
enough to send me an email appreciating it. So, now I have to choose -- Herb and
my peers or some misterious guy you talked to? Hmm, so hard to choose. ;-)

I am not even sure why you insist on bringing that silly "over-complicated" word
here. I am sure it's not complicated for you or the user in general for that
matter. If feels like you are not playing fair -- when it suits you, you play
"dumb" and bring that "over-complication" as an argument; when it suits you
better, you play "smart" and bring some twisted not-even-Pimpl use-case. 

>> 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.
> 
> Please don't go personal this is not apropriate.

LOL. I thought we had turns and you already had yours. 

>>> - No  support of custom implementation classes. Consider
>> I do not understand what you mean  by “custom implementation classes”. 
> 
> I'll explain. Your library restricts that implementation
> class to be:
> 
>   class foo : public pimpl<foo>::value_semantics { ... };
> 
>   template<>
>   class pimpl<foo>::implementation  { ... }
> 
> Sometimes I want it to be different things. My own classes. Not 
> pimpl<foo>::implementation...

That's it? Is it *really* all? I am lost for words, really. Well,
pimpl<foo>::implementation is still *your* class. You implement it. Indeed as an
old and boring person I insist that that implementation is part of the Pimpl
concept and, therefore, should be coded/presented as such (for easier
understanding, self-documentation, maintenance). You, instead, insist on some
other name... any name... but not pimpl<foo>::implementation... is there
something inherently objectionable about pimpl<foo>::implementation or it is
just a rebellion against someone "telling" you what to do?  

>> No need for those “place  holders”.
> 
> I think that I myself and Qt would disagree.
> Placeholders are critical for keeping stable ABI.

I think that I myself and Qt would disagree with your disageement. I deploy
Pimpl with Qt a *lot* as we extend Qt widgets and adapt Qt for our specifics (it
does not play nice in multi-threaded environments).

No-one denies the importance of stable ABI. It's just other people might find
that place-holders idea bizarre and addressable some other more conventional
way. I've been using "my dear" Pimpl for that very purpose. The Pimpl's property
you seemingly deny.

> That is one of the very important use cases of Pimpl in C++.
> 
> Sometimes you want just extensible class and without such
> placeholder you can't add members to object without breaking ABI.

If you want to extend an existing class which size you cannot change, then pimpl
or no-pimpl, place-holder or not, you are stuffed -- you cannot extend the
class. If you can change the size, then instead of extending

struct book
{
    string title_;
    string author_;
}

with

struct book
{
    string title_;
    string author_;
    struct added_data;
    smart_ptr<added_data> added_;
}

I'd do 

struct book
{
    struct added_data;
    smart_ptr<added_data> impl_;
}

where all data including title_ and author_ go into that impl_ which you then
extend as you like.

> Bottom line:
> 
> 1. I don't think Boost.Pimpl flexible enough to be part of Boost
> 2. I don't think Boost.Pimpl's design approach is good enough to
>    be a part of Boost.
> 
> Nothing personal, just a review that expresses my opinion and 
> my experience in this area.

Yes, I think I am done here as well. I can't wait for the real review to get it
over with one way or the other.

V.