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