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

From: Sam Partington (sam.partington_at_[hidden])
Date: 2006-05-15 12:19:22

Before I go into detail - which I'm sorry to say is a bit critical -
I'd like to say that I'm in favour of a pimpl ptr.  Boost's Smart
Ptr's don't quite give what I need. In particular the Smart Ptr
library's shallow-constness changes the semantics of a class when you
introduce a pimpl.

This is a common idiom, and it should be expressed in boost.

> - What is your evaluation of the design?

It seems to be overly complex in some areas.

For example I don't think the comparison operators are going to be
useful enough to justify the extra complexity they cause.  If its felt
that they can be useful, I certainly don't think they need to be
templated on the the RHS type.  A pimpl ptr is used to hide the
implementation of a class, so why would it need to be compared with an
implementation of another type?

Does it actually need policies?  How much use are they?  I'm not
strongly opinioned either way, but it needs to be discussed.

Assuming its decided that policies are needed, do they need to be a
class template?  Can they not be implemented using member function
templates?

The docs claim that pimpl_ptr is extendable through policies, but
since the policy base class (which I would derive from?) is in detail
this would seem to be prevented (well, discouraged anyway).

For example I can think of at least two other policies :

1) Custom deleter. This would also allow the removal of the destructor
from the class that would use it.  And the passing of objects with
pimpls over DLL boundaries.

2) Static storage.  In certain, very rare, cases I can't afford the
overhead of a heap allocation.  But I still want the benefits of a
pimpl.  So a static_storage<size_t Size> policy would be useful.  The
policy would of course check at compile time that Size was correct.
[1]

The first could probably be written, but the second can't be done in
the class template form, because the template template parameters
wouldn't match correctly.

Does it need to be copyable?  or assignable?  I'm fairly sure it
doesn't need to be copy constructible and assignable from a templated
impl_ptr.

> - What is your evaluation of the implementation?

In general it is good, a few minor issues.

Names beginning with _ should be avoided.  Although the standard says
they're ok (17.4.3.1.2), not all library vendors have got it right, so
they're worth avoiding.

Like I said earlier, I don't think the policies should be class templates.

I don't think the copy constructors should be calling T::operator=.
They should call base::base(new T(*rhs_.copy()))

detail::base::get() provides a non-const reference, so it breaks the
const-deepness that operator-> provides.

> - What is your evaluation of the documentation?

The docs appear to have been written in Word.  Using HTML tidy the
docs dropped from 140kb to 34kb.

By document section :

Introduction:  I'm not sure how much detail you need to go into the
whys and hows of the pimpl idiom, can this be mentioned very briefly
with a reference to other literature (such as [1]).  The counter
example - showing why pimpl_ptr helps - does more than the original
example - op== and op!= are added.

I think this section would be better served by showing how using
pimpl_ptr is an improvement over shared/scoped_ptr.  I think the
majority of users that find pimpl_ptr will have already looked at
SmartPtr or at least std::auto_ptr.

Rationale: This section appears to give rationale for the library as a
whole (ie concluding the introduction).  Typically the rationale
provides a rationale for individual (potentially) contentious design
decisions.  Such as : the policy design, including op==, op<.

How it works: I think this section could be removed, it seems quite
obvious to me and it adds nothing that the reference doesn't tell you.

Policy Overview : The exact semantics for each policy need to be
spelled out here.  The current wording is a bit ambiguous.  For
example, the policy manual_creation says "will rely on the user to
create/set the pointer" but nowhere is it specified how you would do
that.

Detailed Semantics:

pimpl_ptr(T* ) in the notes : "Accepts a zero pointer. In this case
the previous pImpl pointer will be deleted."  Surely there cannot be a
previous pointer?  this is a constructor!

operator < : makes a reference to operator!=, I think this is a copy-paste typo.

Type requirements : missed out Assignable, and EqualityComparable, and
LessThanComparable.

> - What is your evaluation of the potential usefulness of the library?

Whilst not a critical library, I think it is a worthwhile one, as its
a common idiom, that should be represented in boost.

> - Did you try to use the library? With what compiler?

No, but I will do when the design is right.

>    Did you have any problems?
n/a

> - How much effort did you put into your evaluation? A glance? A
>    quick reading? In-depth study?

An in depth reading of the code, examples, tests, and docs.  But no
attempt to use it.

> - Are you knowledgeable about the problem domain?

Yes.  I have written a very similar class many moons ago, and I've
been using the pimpl idiom for years.

------------------

My vote is a weak No at the moment.  But if :

+ the documentation was improved
+ base::get() was fixed
+ the exact specification for policies was specified.

Then I would vote a weak yes. And if the following changes were made :

+ policies were changed so they were not class templates.
+ the comparison operators were removed
+ the copy constructor, the assignment operator, and swap were un-templated.

Then I would vote a strong yes.

Sam

[1] Yes I know many, including herb are dead against this technique :
http://www.gotw.ca/gotw/028.htm.  But I still maintain from time to
time it is something I would need/want to do, so it should be possible
for me to write a policy to do that.

[2] http://www.gotw.ca/gotw/024.htm