Re: [boost] [review][pimpl_ptr] Pimpl Pointer review begins

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