Re: [boost] Please look at my suggested pimpl library in the file vault.

From: Asger Mangaard (tmb_at_[hidden])
Date: 2005-10-04 13:10:11

I've uploaded another version with your suggestions. Thanks.

> Rob Stewart wrote
>> > ----------------
>> >
>> > operator=() does shallow copy of dynamically
>> > allocated value. Is this really what ypu want?
>> >
>> It's corrected.
>
> I just downloaded the library and it still does a shallow copy.
> If you intend something else, then you need to rethink how you
> handle copying.

Could you be more specific? I really don't see where it shallows it.

> I think you should have a boost::pimpls namespace.  In that
> you can have the pimpl class and the supplied policies.
I renamed the policies namespace to pimpls. I keep the class itself inside
the boost namespace to shorten the name, eg. boost:pimpl instead of
boost::pimpls::pimpl.

> * The base class should be in namespace detail.
Corrected.

> I suggest a new type, derived from base, called, say,
> "default_construction" that can be the default policy.
Corrected.

> * Why is "PimplType" not just "T?"
Corrected.

> * base::get() error handling should be configurable or nothing.
I've removed the exception, and rely on the user to know how to fix a null
pointer.

> * You m_pPimpl data member name is quite odd.
Renamed to m_pImpl

> * boost::pimpl_policies::base::base() misuses the new operator.
corrected.

> * boost::pimpl_policies::base::~base() should be non-virtual.
Corrected.

> * I'm not sure whether manual_creation should be a separate
>   policy.
>
> Why not have the default policy have a constructor taking a raw
> pointer, defaulted to a null pointer, plus provide a set() member
> function?  You'd need to implement set() just as it is in
> manual_creation now.
>
>    namespace boost { namespace pimpls {
>       template <typename T>
>       struct default_ : base<T>
>       {
> 	 default_(T * pimpl_i = 0) : pimpl_(pimpl_i) { }
> 	 T & get() const { return *pimpl_; }
> 	 void set(T * pimpl_i) { delete pimpl_; pimpl_ = pimpl_i; }
>       };
>    } }
>
> Thus, default_ does what your base and manual_creation policies
> do in one simple policy.

But then you remove the default creation which is one of the advantages of
the library.

> * Don't define empty destructors.
>
> They are noise in the code and can actually be a hindrance to
> optimal code generation.
Corrected.

> * pimpl's member functions should be inlined.
>
> They are all trivial, so you should make them inline.
Aren't templates always inlined?

> * Consider "rhs" or "other" instead of "comparer."
Corrected. I choose rhs.

Thanks for all the suggestions,
Asger Mangaard