Re: [boost] Pimpl Again?

Subject: Re: [boost] Pimpl Again?
From: Gavin Lambert (gavinl_at_[hidden])
Date: 2016-05-30 00:18:49

On 30/05/2016 15:16, Vladimir Batov wrote:
> So, in your view, should we try and have pimpl in Boost? I think we
> should as it'd be useful IMO. I proposed the one I use. I am not married
> to it. If you have a design which makes you happy, let's consider it...
> Otherwise, we won't get far on criticism only without alternatives to
> consider. Don't you think?

As I said at the start, yes, I think this has potential usefulness and 
it would be nice to get something like it into Boost.

I'm just more dubious about the specific design as it stands, so I'm 
trying to open debate about it, with the goal of finding an improvement, 
not shutting anything down.  Perhaps I'm just not very good at getting 
that across.

As it's the template specialisation which causes most of my concern, my 
main query is whether it's really necessary or if it could be structured 
differently to avoid this instead.

> I feel that you make very ambitious generalizations about how users
> will feel and how ugly in your view the code will be of those who
> might decide to use the proposed pimpl.

That's certainly possible.  But perhaps an illustration might better 
explain my perspective (or possibly let others write them off as 
personal coding style issues -- that's possible too).  Taking the Book 
example from the docs (which I know you said are out of date, but it 
seems like the broad strokes still apply) and extrapolating it for code 
that uses a local namespace and with pimpl in the boost namespace, we 
get something like this:

#include ...
#include ...

using std::string;

namespace library
{
     namespace detail
     {
         // some implementation detail definitions, used
         // by the pimpl implementation but not part of it
     }
}

namespace boost
{
     template<> struct pimpl<library::Book>::implementation
     {
         implementation(string const& the_title,
                        string const& the_author)
             : title(the_title), author(the_author), price(0)
         {}
         ...
         bool check_isbn_10_digit() { ... }
         ...
         string  title;
         string author;
         int     price;
     };
}

namespace library
{
     Book::Book(string const& title, string const& author)
         : pimpl_type(title, author)
     {}

     string const&
     Book::author() const
     {
         return (*this)->author;
     }

     ...
}

Namespaces are broken up and repeated, and the implementation of 
check_isbn_10_digit() and other such private methods is outside of the 
library namespace so it can be a bit awkward to refer to things that you 
assume are in scope but aren't actually.  It's also awkward if you don't 
want to define these methods inline for some reason.

You could skip the namespace at the bottom and explicitly implement the 
public methods with library::Book::* instead, but I don't think this is 
an improvement; it just makes return type specification more awkward.