Re: [boost] [pimpl] Mini Review

Subject: Re: [boost] [pimpl] Mini Review
From: Artyom Beilis (artyomtnk_at_[hidden])
Date: 2011-05-29 03:26:32

> From: Vladimir Batov <vb.mail.247_at_[hidden]>
> > 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");
> 
> [snip]
> 
> ... And thanks, man, for not calling me a moron  "EVEN thou" you stand behind
> your opinion. :-) 
> 

Please, don't put words in my mouth that I have never said.

Yes, Boost.Pimpl has holes in it - big holes whether you like it
or not.

So let's stop this "discussion" about what and who had or hadn't said.
It does not bring anything. It does not forward us to a final goal
to have a good pimpl library in boost.


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

I'm aware of this. But don't you want to receive a preliminary reviews to
be ready to hear what would happen on actual review?

The review I had sent wasn't created to "remove" the pimpl from the review
queue or to tell we don't need it, it was written to bring up
real problems or issues or "personal preferences" or what ever you call.

I fully understand that you like your library and your design but I
suggest you to listen to others opinion. It may matter (or not).

Finally, the non-formal-review at this stage only can **help** you
to pass the real formal review as you would know what to expect.

That's it.

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

I glad you do not review KDE's and Qt's code :-)

As I can say - this is your opinion and your way of seeing things, it may
differ from others.

See:

   
http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++#Using_a_d-Pointer


This is common practice.

> >  ...
> > Library does not even provide an option to have a pimpl as  member
> > that is why I think it is inflexible.
> 
> [snip]
> 
> 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).

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

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_;
  };


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_;
  };

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.


Any more arguments?


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


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?

----------------------------------------------

Dear Vladimir,

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.

Best regards and good luck with your library.


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