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

From: Pavel Vozenilek (pavel_vozenilek_at_[hidden])
Date: 2006-05-16 08:16:06

"Rene Rivera" wrote:

> The review of the pimpl_ptr library by Asger Mangaard starts today,

I vote no on the library in current state.
(the problem addressed by David Gruener,
 docs could be much, much better, etc)

I believe there's huge potential if scope
of the library is expanded to provide
"resource management" functionality.

The lazy_creation is interesting start
and I can imagine techniques as:
 * body is self-deleted after timeout when not used
 * pool of objects
 * serialization of objects creation in MT environment
   (I have constructor that cannot be MT safe
    and would like the library to protect against multiple
    constructors/destructors running at the same time.
 * other features suggested bellow.

/Pavel

_______________________________________________
1. The font size of the HTML documentation
   is hardcoded and rather small. What should
   people with large resolutions do?

_______________________________________________
2. The part of introduction explaining pImpl
   should be omitted. Anyone curageous
   to use Boost knows it. Provide a link.

_______________________________________________
3. Color syntax highlighting should be used.

_______________________________________________
4. The funny "C" prefix should be removed
   and the standard "name_with_underscore"
   convention used.

_______________________________________________
5. The policies should not be in detail
   namespace and definitely should be
   documented.

_______________________________________________
6. Description for "manual_creation" policy
   is insufficient and unclear.

_______________________________________________
7. A policy "lazy_creation_with_timeout"
   would be quite useful for real life projects.

Likewise, ability to "un-set" the pointer
manually may be useful.

With these capabilities the library would
go beyond simple pImpl but I think they
would be quite natural here.

_______________________________________________
8. Examples should be moved on the top of the
   documentation and it should be possible
   to copy+cut and compile them.

_______________________________________________
9. Style of the code: the double or triple
   indentation

namespace boost {
    namespace pimpls {
      namespace detail {
          here_we_start

should be replaced with more readable

namespace boost {
namespace pimpls {
namespace detail {

here_we_start

_______________________________________________
10. Inlined function bodies are shorter and more
    readable than out-of-class definitions.

_______________________________________________
11. Naming: "_pDefault": leading underscore
    is reserved for compiler/standard lib.

_______________________________________________
12. In:

  T* p = new T;

  if ( !p )
  {
      boost::checked_delete( p ); <<=== ????
      ....

Why is checked_delete on 0 pointer used?
The type is complete (since constructor exists).

_______________________________________________
13. Style: instead of typing
    pimpl_ptr<T, this_policy> a specialization
    like: lazy_pimpl<T> would be more readable
    and less demanding on scrouring through
    the documentation.

_______________________________________________
14. Test source file contains tabs.

_______________________________________________
15. The pimpl_ptr.hpp is not in boost subdir,
    why should fiddle with directories manually?

_______________________________________________
16. When compiling under BCB 6.4 I got warning
    and error:

[C++ Warning] pimpl_ptr_test.cpp(198): W8004 'oneParameter' is assigned a 
value that is never used
  Full parser context
  pimpl_ptr_test.cpp(104): parsing: int main(int,char * *)
[C++ Error] stl/_algobase.h(75): E2015 Ambiguity between 'boost::void

swap<CMyValues1,boost::pimpls::lazy_creation<T>,CMyValues1,boost::pimpls::lazy_creation<T>

>(boost::pimpl_ptr<CMyValues1,boost::pimpls::lazy_creation<T> > 
>&,boost::pimpl_ptr<CMyValues1,boost::pimpls::lazy_creation<T>

> &)' and '_STL::void 
> swap<boost::pimpl_ptr<CMyValues1,boost::pimpls::lazy_creation<T> >

>(boost::pimpl_ptr<CMyValues1,boost::pimpls::lazy_creation<T> > 
>&,boost::pimpl_ptr<CMyValues1,boost::pimpls::lazy_creation<T>

> &)'

_______________________________________________
17. Compiling and running the test under
    Intel C++ 9.0 plugged in VC6 IDE is OK.

    Also all examples work.

_______________________________________________
18. The manual_creation doesn't show
    why and how this feature could be used.

_______________________________________________
19. I think it is feasible to support
    parameter passing in pimpl constructor.

_______________________________________________
20. The issue addressed by David Gruener
    (incomplete types when using the pimpled
     class in other translation unit)
    needs to be addressed.

_______________________________________________
21. Support for the unsafe and much discouraged
    hackery with fixed size buffer where the
    body is constructed would be useful at a times.

    Support of static storage for the
    body may be useful for those
    giant classes usually named "toolkit".
    The library may contain code checking
    that just single instance of the body is ever created.

_______________________________________________
22. The feaures suggested by Tobias Schwinger
    would be very handy.

_______________________________________________
23. Possible feature: provide support for
    several bodies that are composed into single
    unified interface:

    class handle {
    ...
     body1* pimpl1;
     body2* pimpl2;
     ...
   };

to:

   class handle {
    ....
    pimpl_ptr<struct Body1, struct Body2> pimpl
   };

    void handle::foo() {
      pimpl[0]->foo();
    }

_______________________________________________
24. Feature: provide ability to "switch" class
    type dynamically (as if virtual table is replaced).

    The "set()" could be such feature but it is not
    documented enough and the name is not descriptive.

    The switch should be possible from within
    the body (the most common scenario).

_______________________________________________
25. Feature: provide wrapper that intercepts
    certain exceptions and switches to callback.

Example: two body implementations are provided.
If function from body1 throws this or that
exception it is intercepted, ignored and instead
result of call into body2 is returned.

_______________________________________________
26. Feature: provide well documented framework
    so people could add their own specialised
    pimpls (e.g. every method runs in its own thread,
    if timeout expires a default value is returned).

The features suggested above could be used
as examples of such framework.

_______________________________________________
EOF