
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