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