From: Cliff Green (cliffg_at_[hidden])
Date: 2005-10-27 16:05:28
I use the pimpl idiom quite a bit and have followed the
discussion on this proposed library. Here's some comments
from looking at the just uploaded version (I didn't look
at any of the previous versions in any kind of depth):
1. The Motivation and Rationale sections of the
documentation needs to be much more detailed,
comprehensive, and clear before I will use this library.
To me, this is crucial for this library, since there must
be clear advantages over existing practices and techniques
(I think this is true of most "idiom" based libraries,
versus libraries fulfilling a specific functionality
need).
-- Summarize Sutter's treatment of pimpl's, and compare
and contrast some of the existing pimpl approaches.
Provide a reference / link to Sutter's articles and
books(http://www.gotw.ca/publications/mill04.htm).
-- Summarize some of the disadvantages: performance will
decrease (methods cannot be inlined, additional heap
allocation and deallocation is required, an extra
indirection is required for each method invocation) and
there will be increased heap usage (which can be an
issue for embedded systems).
-- One of the primary uses for pimpl's in my experience is
in hiding 3rd party or OS headers from the application
using my class. This advantage should not be
underestimated / underdocumented, specially for large
system development, or for writing libraries that will be
used on multiple platforms / environments.
-- Contrast your library with using boost::shared_ptr or
boost::scoped_ptr - in particular, both shared and scoped
ptrs support incomplete type usage (in the header). Right
now, the only advantage I see in using pimpl_ptr versus
shared / scoped ptr is in the policy behavior - lazy
creation, etc. Will this advantage go away once/if a
policy ptr is accepted into Boost?
2. Additional examples would be very helpful, and
shouldn't be too hard to produce. The two existing
examples should have some comments and / or additional
code to point out the advantages of using pimpl_ptr.
3. A documentation re-write for improved grammar and
writing style is needed.
4. "In general" doesn't seem precise for the type
requirements. Either type T must be copy constructible or
not, or specify the requirements by pimpl_ptr methods /
use. The tests should enforce the requirements. Also,
looking at the code, there's at least a couple of areas
where assignment is required for the contained type T
(e.g. pimpl_ptr::operator=) and "operator==" is required
for the contained type in at least two places.
5. Looking at the code, the pimpl_ptr copy constructor is
performing assignment in the ctor body ... how is the deep
copy being performed? I don't see deep copy semantics in
either the "base" class or the policy classes. Does this
affect the requirements of the contained type T?
6. If a pimpl_ptr is default constructed with a
manual_creation policy, how is the impl object created /
passed in at a later time?
7. One of the issues with policy based template classes is
compatibility / conversion between types with different
policies. In this library, pimpl_ptr<Foo> and
pimpl_ptr<Foo, lazy_creation> are different types, and
it's not clear to me how / if they can interact with each
other.
8. Swap is not swapping policies, only the underlying impl
pointer (unless I'm confused).
Hope this helps,
Cliff