Re: [boost] Review Request: pimpl

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