Boost logo

Boost :

From: Ulrich Eckhardt (doomster_at_[hidden])
Date: 2006-05-25 03:10:37

On Monday 15 May 2006 02:45, Rene Rivera wrote:
> Direct download at <>.
> - What is your evaluation of the design?

Read through it, no in-depth analysis. I mostly like it, in particular that
there is a ready-made implementation for lazy initialisation.

> - What is your evaluation of the implementation?

Read through it, no in-depth analysis. There are a few things that struck me:
- There are several memberfunctions or class detail::base that are declared as
inline and outside the class body, while others miss this. Why not declare
them inside the class, so they get the inline implicitly? Less typing, less
reading, easier maintenance, same binary code.

- I saw this code:
  T* p = new T;
  if ( !p ) {...}

but new will throw when allocation fails. IFF something like that was done for
broken compilers/standardlibraries, then it should be documented. I'd even
say it should then go into a boost-wide systematic workaround, encapsulated
in a macro that evaluates to nothing in conformant environments.

- Examples use FOO__H as include guard, which is invalid because it contains
two consecutive underscores.

- I wouldn't provide operators !=, == and <. These need manual intervention to
forward them to the pimpl anyway and then it is just a little step from
writing "return lhs.impl==rhs.impl;" to "return *lhs.impl==*rhs.impl;".

- I wouldn't provide the templated constructors and operator. Someone already
mentioned that this makes no sense when the implementation is hidden, which
includes hidden from other implementations. Anyhow, what other
implementations would you want to compare with? If it's the same type, it
doesn't matter, if a class has different implementations, I wouldn't exactly
call this a PIMPL.

- operator* is missing.

- Could use a safe_bool conversion, in particular for delayed initialisation.

> - What is your evaluation of the documentation?

- The transitivity of const is IMHO another goal of such a library, i.e. that
a const pimpl only allows const access to the implementation.

- A repetition of the assumption that new could return null.

- Calling an object 'uninitialized' after its constructor ran is confusing
("Postconditions: *this is uninitialized.").

- Documentation should mention that all functions that need to know the full
type of the implementation (i.e. ctor, dtor..) must not be inline. Examples
should reflect this, there is no use for a PIMPL when everything is in a
single file anyway.

- The use of an on-demand created object in vector it related but still IMHO
completely outside the scope of a PIMPL library. I would remove this use from
the design goals and documentation and even make the pimpl_ptr noncopyable.
If the final design still say it can be used for this, I would also mention
the alternative of using Boost.Optional, which acts similar.

> - What is your evaluation of the potential usefulness of the library?

Medium, though I must underline the 'potential' in this statement. The current
state needs some more work. Other than that (and the reason for the 'medium')
PIMPL is just too simple to get right, in particular since often these
complex classes are entity classes and as such noncopyable, reducing the
effort to ctor and dtor.

> - Did you try to use the library?


> With what compiler?


> Did you have any problems?


> - How much effort did you put into your evaluation? A glance? A
> quick reading? In-depth study?

Around 90 minutes of reading the library and writing this mail.

> - Are you knowledgeable about the problem domain?

I used the PIMPL pattern before and followed discussions on the topic.


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