Hello All, Because a pre-review runs this days in collaborator tool I still want to provide some feedback on this library I was looking on long time ago. - What is your evaluation of the design? --------------------------------------------- This is the major problem of the library: It tries to do simple things overcomplicated. --------------------------------------------- Take as an example: struct Book : public pimpl<Book>::value_semantics { ... }; Is an attempt to do things in "fancy" way where it is not needed. struct Book { public: private: struct implementation; smart_ptr<implementation> pimpl_; }; Is much clearer. The proposed solution is overcomplicated hard to read and in general makes more troubles then helps, makes the code less readable by average programmers. ------------------------------------ shared_ptr as pointer model is Wrong ------------------------------------ You almost never want to have shared object semantics for pimplized object. I use pimpl ideom widely in my projects CppCMS, CppDB and even in Boost.Locale and I used shared semantics only once where it was explicitly shared. In rest of the cases I had found myself using following smart pointers: 1. copy_ptr - pointer the copies underlying object on its copy. i.e. something like copy_ptr(copy_ptr const &other) : p_(new Object(*other)) {} Copy the object on copy. 2. clone_ptr - pointer that clones underlying object on its copy. i.e. something like copy_ptr(copy_ptr const &other) : p_(other->clone()) {} 3. hold_ptr - pointer with same const-semantics as an object it points to T const &operator*() const; T &operator*() ; T const *operator->() const; T *operator->() ; They cover almost all use cases, so with addon of shared_ptr it covers ALL use cases. In any case current boost::scoped_ptr or std::auto_ptr is much more suitable smart pointer for implementing pimpl then Boost.pimpl. The only advantage I can see in boost.pimpl over scoped_ptr or auto_ptr is that it does not require explicit destructor. i.e. class Foo : boost::noncopyable { public: Foo(); int x() const; void x(int v); private: struct data; std::auto_ptr<data> d; }; No good. You need to add ~Foo(); So it would know to destroy Foo::Data correctly. But this can be easily solved on hold_ptr, copy_ptr and clone_ptr level same as it solved for shared_ptr and in reality even this not needed as it is better to add empty destructor in cpp. Spare Pointer use case ------------------------ Another important use case in the case when pimpl is used to handle ABI stability and in general the object has spare "pimpl" or d-pointer for future uses. Consider: struct book { public: book(); book(book const &); ~book(book const &); book const &operator=(book const &); std::string title() const; std::string author() const; void title(std::string const &); void author(std::string const &); private: std::string author_; std::string title_; struct data; copy_ptr<data> d; }; Now in book.cpp we define data as struct book::data {}; And we do not initialize d at all. It is fast and efficient object that does not use data at all. Now when we decide to add new member "isbn" without breaking ABI. in book.cpp we define in book.cpp struct book::data { std::string isbn_; } And now we initialized the d pointer in constructor. And add new members in book.h std::string isbn() const void isbn(std::String const &); And implement them std::string book::isbn() const { return d->isbn_; } void isbn(std::string const &s) { d->isbn_ = s; } This is very important use case the the author does not even relate to. - What is your evaluation of the implementation? Generally not looked into too deeply but it is does not follow boost standards: - Naming conventions - Documentation - Build So it is not even ready for the review (even thou it can be easily changed as it is a small library) - What is your evaluation of the documentation? 1. The documentation should be in HTML file not on some Internet web site. 2. Rationale part missing The documentation in not Boost-Ready - What is your evaluation of the potential usefulness of the library? Useless for 90% of pimpl use cases. - Did you try to use the library? With what compiler? Did you have any problems? No I hadn't - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? About 4-5 hours of studying - Are you knowledgeable about the problem domain? Yes, I work with pimpl-ized object on daily bases in many different approaches including implementing my own smart pointers for pimpl (clone_ptr, hold_ptr, copy_ptr) - ABI Stability - Compilation time reduction, - Decoupling of the interface from implementation details. 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. Best Regards, Artyom Beilis -------------- CppCMS - C++ Web Framework: http://cppcms.sf.net/ CppDB - C++ SQL Connectivity: http://cppcms.sf.net/sql/cppdb/