Re: [boost] Review Request: pimpl

From: Asger Mangaard (tmb_at_[hidden])
Date: 2005-12-19 04:51:03

After almost two months I've finally uploaded an updated version.

> Cliff wrote:
>
> 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).
I've tried hard to meet these requirements in the update.

> -- 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?
I've included an example called 'compare_to_other.cpp' which should show
the advantages compared to other smart pointers. Should a policy_ptr be
introduced to boost, I'll have to revise my work.

> 3. A documentation re-write for improved grammar and
> writing style is needed.
I'm from Denmark, and English isn't my first language. Still I'm sorry if
my english is really that bad :(

> 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.
I'm not sure what you mean here. The examples given should very clearly
show what is required from the pImpl class.

> 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?
Deep copying is performed by converting the pImpl pointer to a reference,
and copying.

> 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?
By using 'set'.

> 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.
Since all policies implement the 'get' function, all relevant operations
can be performed.

> 8. Swap is not swapping policies, only the underlying impl
> pointer (unless I'm confused).
That's correct. You cannot swap policies. That's how C++ works (unless we
recreate each pimpl_ptr, etc. but that would require more work than
perhaps desired by the user).

> Hope this helps,
It helped a lot. Thank you.

Regards,
Asger