Re: [boost] [review][pimpl_ptr] Pimpl Pointer review begins

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 <http://tinyurl.com/m66ox>.
[..]
> - 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?

Notes:
- 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? 

No.

> With what compiler? 

N/A.

>    Did you have any problems?

N/A.

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

Uli