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

From: Rob Stewart (stewart_at_[hidden])
Date: 2005-10-04 11:24:02

From: "Asger Mangaard" <tmb_at_[hidden]>
> >> Pavel Vozenilek wrote ...
> >
> > Closing bracket for namespace should nopt have ";"
> > but should have comment like // namespace pimpl_policies
> >
> It's corrected.

"Should" is a strong word there.  I dislike such comments because
they too easily get out of sync with the start of the block.
Some like that style, some don't.

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

___________________________________
I have some of my own comments now.

* Why the pimpl_policies namespace?

I think you should have a boost::pimpls namespace.  In that
you can have the pimpl class and the supplied policies.

* The base class should be in namespace detail.

The name suggests that this type isn't one clients of the library
should be using.  However, the behavior of the class suggests
that you really want it to serve two purposes (also evidenced by
its being the default pimpl policy type).

I suggest a new type, derived from base, called, say,
"default_construction" that can be the default policy.  That
class can construct an instance in its default constructor and
provide the get() member function you currently have in base.

Once you do those things, then base can have a constructor taking
a raw pointer to the impl object and you can eliminate the get()
member function.  At that point, base will have only one purpose
instead of the two it now has.

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

The whole point of the template is to house a pimpl type, so the
added verbosity doesn't seem helpful.

* base::get() error handling should be configurable or nothing.

Throwing an exception isn't always the appropriate action.  Some
would prefer getting a segfault and a core dump, for example.
Either omit the test of m_pPimpl or control it via another
policy.

* You m_pPimpl data member name is quite odd.

The reason this is the "pimpl" idiom is because of the common "p"
prefix for pointers to an implementation object.  That leads to
"pimpl_," "m_pImpl," "_pimpl," and other variations, depending
upon your naming scheme.  Using both the "p" prefix and the "P"
in the data member name is redundant.  My preference is "pimpl_,"
but "m_pImpl" is probably yours.

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

"new PimplType()" is incorrect.  You want "new PimplType."

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

No one should be creating polymorhpic instances of the pimpl.

* Your explanation of the mutability of m_pPimpl is redundant.

That's the reason mutable exists.  You might want to be more
specific as to the function that uses it or just remove the
comment.

* Since lazy_creation and manual_creation derive from base,
  you'll cause warnings about hiding base::get().

base should not have a get() member function.  The derived
policies should have to implement get().

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

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

* Consider "rhs" or "other" instead of "comparer."

This is simply a stylistic suggestion which isn't terribly
important, but your use of "comparer" stood out.  First, what
you've called "comparer" isn't comparing.  Second, it's a rather
long name for something so straightforward.

-- 
Rob Stewart                           stewart_at_[hidden]
Software Engineer                     http://www.sig.com
Susquehanna International Group, LLP  using std::disclaimer;