[boost] [pimpl] Preliminary Review

Subject: [boost] [pimpl] Preliminary Review
From: Gregory Crosswhite (gcross_at_[hidden])
Date: 2011-05-26 22:07:30

Hey everyone,

Here are my preliminary thoughts regarding the pimpl library.

First, I like the idea of having it or something similar to it be part 
of Boost because the functionality seems like it would be useful in a 
lot of cases.  One of the strengths of including such a library in boost 
in my opinion is that it will appear in the "Libraries" list, which 
means that more people will be exposed to the idiom and will think of it 
as a possible option than they would otherwise.  Put another way, Boost 
is both a collection of libraries and of best practices, so a good pimpl 
implementation belongs in boost even if it is a relatively small one.

Now the question is whether this particular implementation itself is 
good enough.  Honestly, I have trouble figuring out exactly what it does 
except by reading through the code since the documentation itself 
doesn't say what it does behind the scenes except to say vaguely that it 
implements "pointer semantics" or "value semantics".  So one thing that 
the documentation really needs (among the many others that have been 
mentioned) is a section that clearly lists the features that it offers.

One objection that Artyom reasonably raised is that this implementation 
may try to do too much, and doesn't offer much in the way of 
customization.  However, people like Artyom are most likely going to be 
rolling their own implementation of the pimpl idiom anyway.  I think 
that it is sufficient that a implementation be "good enough" for an 
ordinary user's needs that they won't have to think about any of the 
ugly details since it does all of that work for them, and to the extent 
that this is true of the current implementation

Now for my thoughts on the code itself.  Two of the classes in the 
implementation should probably be reviewed separately and put in 
different Boost libraries rather than the Pimpl library, namely safebool 
and value_semantics_ptr.

safebool is an interesting solution to the problem that implicit 
conversions to the "bool" type implies implicit conversions to hosts of 
other types such as integers which can result in all sorts of  
unexpected behaviors that will bite you when you least expect it.  
safebool solves this problem by providing a convenient way for you to 
implement an effective "bool()" by instead implementing an operator that 
returns a special function-pointer that is specialized to your class and 
thus is equal neither in type nor value to a safebool for any other 
class;  a minor cost that is presumably incurred by this is that a small 
amount of code will be added to the binary for this special no-op 
function, since I don't see how this can be optimized away.  The 
implementation looks straightforward and the comments contain good 
documentation explaining both the usage and the rationale.  I would 
recommend that safebool be accepted into boost and added to the 
utilities library on its own merits.

value_semantics_ptr is a class that contains a pointer to data (or 
possibly a null pointer) and has the semantics that upon assignment it 
copies the data from another pointer, allocating (or freeing) its 
internal pointer if necessary.  I think that this is a good idea and 
that something like this belongs in boost, but I suggest that the 
current implementation should be changed at least as follows before 
being accepted:

First, it should be taken out of the pimpl<> class and made independent.

Second, the traits struct member should be changed from a derived class 
to a second parameter in the list of template arguments to the 
value_semantics_ptr type, with the deep_copy member being pulled outside 
and being made the default value of the new second template argument.  
Alternatively, a type function --- e.g., copy_traits<T> --- could be 
used instead.

Third, if Boost::Move is going to be ready soon it would be ideal if 
this class could be modified to support it in order to potentially save 
some extraneous copies.

Fourth, one of the methods confuses me and apparently also the author:

     bool operator<(value_semantics_ptr const& that) const
     {
         return this->impl_ < that.impl_;
         // Should it be "*this->impl_ < *that.impl_" instead?
     }

so this should be finalized.

Fifth, the implementation will need to be fleshed out.  operators such 
as * (dereference) and -> will need to be added, as well as some 
additional typedefs such as value, pointer, reference, etc.  Operator 
safebool() will also need to be added.

In conclusion, if pimpl is accepted I think it would be better for there 
to be a polished value_semantics_ptr that we feel comfortable having be 
directly tested and exposed to users be the back-end of this class than 
something hidden away, so when value_semantics_ptr is ready I think that 
it should ultimately be accepted to the Smart Ptr library.

So much for safebool and value_semantics_ptr, now on to pimpl itself.

The code looks straightforward and the comments document it reasonably 
well --- in fact, the ratio of comments to code in the main pimpl class 
is roughly 2:1, and in that count I was lazily including whitespace and 
single { and } lines as code lines, so that gives you a sense of how 
heavily it is commented.  I have only taken a quick glance through it 
since this is a pre-review, but it all looks simple and straightforward 
to me, and it does a good job of generating tedious boilerplate so the 
user doesn't have to, exactly as it claims.  On a side note, one 
additional set of semantics that I would recommend adding is move 
semantics, which could be implemented using the Boost::Move library.

In conclusion, I think that this library should be broken into parts 
with safebool being put in utilities, value_semantics_ptr (and we can 
bikeshed over the name here of course :-) ) being put in smart ptr, and 
pimpl being put in a separate library.  After this is done pimpl will be 
quite small, but I think that it pulls enough weight that it is 
nonetheless useful.  To repeat what I said earlier, boost is not just a 
repository of libraries but of C++ best practices, so even if pimpl is 
small if it does a good job of capturing the pimpl idiom I still think 
that it should be accepted into Boost.

Cheers,
Greg