Boost logo

Boost :

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

> * The base class should be in namespace detail.

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

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

> * 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

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

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

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

> * 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.

> * 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

Boost list run by bdawes at, gregod at, cpdaniel at, john at