Re: [boost] [pimpl] Mini Review

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

> Artyom Beilis <artyomtnk <at> yahoo.com> writes:
> ...
> I glad you do not review KDE's and Qt's code 

I am glad I do not have to. I've seen it. It's not exactly pretty. It's somewhat
understandable as their code base has evolved over quite considerable period.
Say, their signal/slot idea is so much better implemented using boost::signal
rather than using some obscure macros, qt-precompiler, run-time detection of
incorrect signatures. It does not make the solutions they had to resort to
common practice though.

> See:
>    
>
http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++#Using_a_d-
Pointer
> 
> This is common practice.

>From that URL: The d-pointer pattern has been described many times in computer
science history under various names, e.g. as pimpl, as handle/body or as
cheshire cat. 

So, as you can see Pimpl *is* a d-pointer. It beats me why it's so important to
support another d-pointer inside Pimpl (which by author's description is a
d-pointer). 

>> The library addresses a certain  well-defined concept/issue. It is about
>> constructing a Pimpl class easily,  safely and painlessly. 
> 
> It is ok, but all I'm trying to say (may be in 5th time) that pimpl
> is little bit more then that.
> 
> It is ok that Boost would have the "pimpl" template you suggest
> but it should not be "Pimpl" library but a part of pimpl library.
> 
> Pimpl - is much wider topic (IMHO).

I disagree that Pimpl is "much wider topic". The idiom is well-described and
well-defined in various sources. I think in the original article I mentioned
sources which I based my understanding of Pimpl on. I believe I was faithful to
the last letter to those sources and to the Pimpl idiom definition. Could you
please list the sources where you get your extended interpretation of the Pimpl
idiom from? Pimpl is essentially another smart pointer and all of them
(shared_ptr, unique_ptr, etc.) have their well-defined qualities, properties and
applications. If we start arbitrarily "extending" those definitions and their
applications beyond what those pointers were designed for, we'll end up with a
mess.

> ...
> I had shown you 2-3 examples of such cases where it is not suitable but
> 
> I'll repeat:
> 
> a) Wrapping different implementation (not mine) with different interface
> 
>   class ustring {
>   private:
>      smart_ptr<icu::UnicodeString> pimpl_;
>   };

With my Pimpl it's done as follows:

// Interface
struct ustring : pimpl<ustring>::whatever 
{ 
    pure interface;
};
// Implementation
template<> pimpl<ustring>::implementation
{
    icu::UnicodeString data;
}
or
template<> 
pimpl<ustring>::implementation : public icu::UnicodeString
{
}

> b) Improving compilation speed by hiding a real object behind
>    a lighweight wrapper.
> 
>   //
>   // Real implementation uses havy classes in headers like Asio.
>   // used in many locations,
>   //
>   class connection {
>   public:
>     ...
>   private:
>     boost::asio::ip::tcp::socket socket_;
>     ...
>   };
> 
>   //
>   // Lighweight wrapper for basic functionality that is
>   // used in places where I don't want to include full ASIO.
>   // to do very simple stuff where connection is the real
>   // fully functional object.
>   //
>   class connector {
>   public:
>     ...
>     void setip(std::string ...)
>     connection &impl();
>     ...
>   private:
>     smart_ptr<connection> pimpl_;
>   };

With my Pimpl it's done as follows:

// Interface
struct connector : pimpl<connector>::whatever 
{ 
    pure interface;
};
// Implementation
template<> pimpl<connector>::implementation
{
    connection data;
}
or
template<> 
pimpl<connector>::implementation : public connection
{
}

> c) Not calling class "implementation" just because it only
>    holds data memebers and not real implementations (AKA d-pointer)
> 
>    Only to make adding new members not to change its sizeof
> 
>    struct book {
>    private:
>      // data members - not functions.
>      smart_ptr<data> d; 
>    };
> 
>    So having pimpl<book>::implementation is misleading as it
>    is only... discuonnected part of body - have data members.

Applying Pimpl idiom to the book above is, uhm... err, ...makes no sense to me.
I'd suggest

// Interface
struct book : pimpl<book>::whatever { interface; }
// Implementation
template<> pimpl<book>::implementation : public data {}
 
> Any more arguments?

No, I am just tired. Let's just agree to disagree. 

> Consider this case:
> 
> statistics.h
> 
>   class statistics {
>   public:
>     ...
>     void add_new_value(double v);
>     double std() const;
>     double mean() const;
>     ...
>   private:
>     // enough to handle standard deviation and mean values
>     int N_;
>     double sum_; 
>     double square_sum_;
> 
>     // Place holder for future use
>     // for statitics that can't be calculated with
>     // only the values above.
>     struct data;
>     smart_ptr<data> d;
>   };
> 
> statistics.cpp
>   ..
>   void statistics::add_new_value(double v)
>   {
>     N_++;
>     sum_+=v;
>     square_sum_ += v*v;
>   }
>   ...
> 
> In this case you keep a smart pointer just as place holder not in use,
> the class is generally simple value type but you want to keep it extensible.
> 
> This class does not even require memory allocation (at this point)
> But once when a new type of statistics is added the "d" pointer
> would be in use.
> 
> Is this valid use case or not?

Yes, it is a valid use-case... poorly implemented. I'd apply normal Pimpl idiom,
i.e. I'd separate interface from the data/implementation; I'd hide the
implementation and that includes 
     int N_;
     double sum_; 
     double square_sum_;

like

template<> pimpl<statistics>::implementation
{
     int N_;
     double sum_; 
     double square_sum_;
};

Then I'd extend pimpl<statistics>::implementation when my requirements change.
In fact, I already do exactly that. If I want to avoid memory allocation, then,
I'd use a policy as Gottlob Frege suggested... but I'd keep design clean and
simple.

> I'm just trying you to show many other use cases that I had met,
> used, deployed in my own and not-connected-to-me-at-all software.
> 
> You provided a nice pimpl framework but *in my opinoin* 
> it does not cover enough use cases. If a real formal
> review was running these days, you would get my negative vote.

Yes, I see and understand your position and your needs (they are not really that
different from mine -- I face similar issues). We happen to approach things
differently. And that's OK. It's equally OK if you vote yes, no, whatever. In
all honesty I do not care who and how votes... and I won't write your name into
the list of my enemies. If fact, I kinda like you (even though you've been
getting on my nerves lately :-) ). Equally I do not really care if Pimpl is
accepted into Boost or not. Quite some time back I thought I had something
simple and useful for others. So, I was willing to share it with others. If
people vote it out, then they do not need it. And that's fine by me. Now I am
just tired and want to get it over with one way or the other.

And, yes, I admit I've been quite nasty to you lately. My honest and humble
apologies. My only excuse could be that "you started it"... but I am a bit old
to use that line of reasoning. So, I have no excuses. I am sorry. I wish you all
the very best and I wish your pimples soundly beat mine. ;-) 

V.