Rene Rivera wrote:
- Are you knowledgeable about the problem domain? - What is your evaluation of the potential usefulness of the library? - What is your evaluation of the design?
As someone who uses the handle/body idiom quite frequently, I'd say it can be very useful. But o for noncopyable classes (90% of the "pimpl candidates" in my projects) scoped_ptr works fine o in other places shared_ptr or intrusive_ptr work, but needs extra work to implement deep copy plus with shared_ptr there is unecessary overhead for reference counting and with intrusive_ptr it needs two no-op functions to bypass reference counting. Brings us to the question: What can pimpl_ptr give us that Boost.SmarPtr can't? The answer to this question with the current state of the library is: o save a few lines of code for deep copy o save a few lines for "deep compare" (it seems unlikely to me that this feature will be very useful) Next question is: What could an imaginary pimpl_ptr give us that Boost.SmartPtr can't? o stack allocation using SBO o COW (copy on non const access) o both of the above (these are the features I had actually hoped for) I have a bad feeling about the policies, especailly the first two of them. "lazy creation" could be interesting, but I'm not too sure about that.
- What is your evaluation of the documentation?
I'd prefer body_ptr for the name, because: o it makes some sense to those who never heard about the pimpl idiom o it sounds much sexier ;-) The layout does not really hit my taste, that is I find it hard to read. Especially the reference needs to be spaced out. Further, I don't like the upper case names used in the examples (I guess it's just a matter of taste, still, it's Boost style). The code in the tutorial can probalby use some abbreviation. And I think you should compare the library with other Boost smart pointers rather than with hand-written code. boost::pimpl_ptr<struct CPrivateMembers*> m_pImpl; in the client class, where pimpl_ptr obviously adds another pointer to T pimpl_ptr( T* _pDefault); seems like a typo (one that can cause serious confusion).
- What is your evaluation of the implementation?
Didn't look.
- Did you try to use the library? With what compiler? Did you have any problems?
No.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
A quick reading.
Please always explicitly state in your review, whether you think the library should be accepted into Boost.
I vote not to accept this library, at lest not in its current state. Regards, Tobias