"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