From: Tobias Schwinger (tschwinger_at_[hidden])
Date: 2006-05-15 06:02:15
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