Subject: Re: [boost] [pimpl] Mini Review
From: Artyom Beilis (artyomtnk_at_[hidden])
Date: 2011-05-26 08:47:17
> From: "Stewart, Robert" <Robert.Stewart_at_[hidden]>
> Thomas Klimpel wrote:
> > Artyom Beilis wrote:
> > > Should The library be accepted in Boost:
> > > ========================================
> > >
> > > No it should not. It is "a fancy" library that was written
> > > without a knowledge of the problem domain. It tries
> > > to solve problem in smart way making it overcomplicated.
> > >
> > > IMHO old std::auto_ptr is much more suitable
> > > for writing pimpl-objects then Boost.Pimple.
> > >
> > > For easy pimpl-ideom you do not need a fancy class
> > > but rather a set of pimpl-suitable smart pointers.
> >
> > I haven't reviewed the Pimpl library, but this comparison to
> > "old std::auto_ptr" looks very strange to me. If I understood
> > correctly, one of the reasons why "old std::auto_ptr" is
> > deprecated in favor of "std::unique_ptr" is that "old
> > std::auto_ptr" will silently break the default generated copy-
> > constructor and assignment operator of a class containing an
> > "old std::auto_ptr" as member. "std::unique_ptr" on the other
> > hand will disable default generated copy-constructor and
> > assignment operator and only default generate the "move"
> > versions of these, so the user will at least realize that he
> > has to manually provide correct versions of copy-constructor
> > and assignment operator.
>
> It is these issues and the several manual steps required to implement the
>Pimpl
> Idiom that this library tries to address and simplify.
>
Simplification does not imply that user must derive its own class from
other class?!
It does not simplifies things, it makes them more complicated.
Consider following copy_ptr that both safe and simple.
template<typename T>
class copy_ptr {
template<typename O>
explicit copy_ptr(O *p) : ptr_(p), deleter_(deleter<O>),copy_(copy<O>) {}
copy_ptr(copy_ptr const &other) :
ptr_(0),
deleter_(other.deleter_);
copy_(other.copy_)
{
ptr_ = copy_(other.ptr_);
}
~copy_ptr() { if(ptr_) deleter_(ptr_) }
...
private:
T *ptr_;
void (*deleter_)(T *p);
T *(*copy_)(T const *p);
template<typename O>
static void deleter(T *p) { delete static_cast<O *>(p); }
template<typename O>
static T *copy(T *p) { return new O(static_cast<O const &>(*p); }
}
It is safe for use by total newbie, but it does not require
from a user to derive its own class from other one.
The proposed solution is too complicated and overkill.
It does not makes things simpler.
> It is totally clear that one can use shared_ptr, unique_ptr, etc.
> or even a raw pointer to implement the Pimpl Idiom.
>
> However, each such choice requires different member functions to effect proper
>copy and equality semantics,
With copy_ptr above no problem with copy semantics or other semantics.
Also note Boost.Pimpl's equality semantics is wrong.
You don't compare pointers to implementations but implementations themselves.
So
operator==(Book const &other) ( impl_ == other.impl_ /* as pointer */ }
Is wrong, you need to compare *impl_ == *other.impl_
And it is not possible to do in header file as actual implementation required.
> It may be that Artyom has used Pimpl so often that he never makes mistakes
> in its implementation for each use case. He may understand the issues
> completely and may always do the right thing. That doesn't mean that
> others will do as well. Libraries exist to capture design and
> implementation details so others can build upon that effort,
> even without fully understanding the issues.
>
That is why I had created a set of smart pointers for pimpl.
It has nothing to do with making mistakes. That is why copy_ptr solves the
problem
and hold_ptr (that is just non-copyable copy_ptr) solves the problem as well.
They are safe but also simple.
Simplicity is the key. Derivation from other class is not simplicity.
Writing
template<>
class pimpl<foo>::implementation
Over
foo::implementation
Is not simplicity.
Especially when it limits your to very specific type when you may hide some
other existing complex type that you may just not show.
> It may be that this library needs to be extended to support
> cloning, holding, and other use cases the author hadn't identified.
>
> That hardly justifies saying that the author does not have "knowledge of the
>problem domain."
I'm sorry if it seems rude. However, some design issues in the
library design make me feel that too many important uses cases
are just not covered, that's it.
For example, consider something like
xml_document {
public:
...
private:
smart_ptr<xmlDocPtr> pimpl_;
}
Very popular type use case when one class if a facade
of other complicated one or one that is does not exposed
to user.
Can't be done with Boost.Pimpl as pointer to implementation
is restricted to specific type.
The problem is that the library solves only one particular
use case and does not see too many other very popular
use cases existing in the real software.
Artyom Beilis
--------------
CppCMS - C++ Web Framework: http://cppcms.sf.net/
CppDB - C++ SQL Connectivity: http://cppcms.sf.net/sql/cppdb/