On Jan 14, 2008 12:10 PM, John Torjo <john.groups@torjo.com> wrote:
* What is your evaluation of the design?
Not generic enough. It addresses a number of specific problems (e.g. use in MT contexts, order-of-initialisation fiasco, etc) but the author's design choices are neither customizable nor overridable. For instance, the design could be made more flexible by allowing thread-safety to be specified as a policy (with appropriate default) or by delegating the responsibility to the wrapped type. If BOOST_HAS_THREADS is defined, it looks as though I have no choice but to use mutexed instantiation and/or access. If I'm not interested in synchronised access, then the lease interface is useless. The solution to the OOI problem does not scale, and I would go as far as saying that it doesn't work in practice. In my experience, it's a constant maintenance nightmare to try and keep the various slot assignments in the correct order (I have to deal with a large number of singletons; yes that is a problem in and of itself - this library will do nothing to discourage it). The only scheme I've personally had any success with, so far, involves some form of reference counting, i.e. singletons that depend on other singletons do so explicitly (by incrementing the other's refcount on construction; the singletons are destroyed when their refcount goes to zero). Another reason why the proposed OOI solution fails in practice is that other singletons are not a singleton's only dependency: client code ( e.g. objects that reference the singleton) are dependencies as well and may affect the order of destruction. You just can't assume the singleton manager will be the last thing to go - there may be other globals. BTW I find the very idea of being able to "force" singleton destruction quite scary, but I haven't looked at the implementation details. The following sentence is also cause for concern: " Attempts to access an already destroyed Singleton instance cause the instance to be re-created." Is that true only for DLL usage? My biggest problem with this design is that it is unsuitable for library writers: types derived from typical singleton class designs, including boost::singleton and friends under review, invariably give rise to untestable code. To make a singleton's client code testable involves exposing some kind of overridable factory so that the singletonized type can be replaced with a test mockup. So far I have found this to be a hard problem to solve in general. Finally, this may constitute a minority opinion but I like to consider global access and single instance as orthogonal concepts. One may want to ensure a resource is only instantiated once without necessarily providing global access to that resource.
* What is your evaluation of the implementation?
Unnecessarily complex as a consequence of the design. If the functionality was layered, the code would be more readable. * What is your evaluation of the documentation?
It's OK although the DLL bit is unclear to me.
* What is your evaluation of the potential usefulness of the library?
I won't use it, but I command the author's attempt to provide a library for such a controversial design pattern. * Did you try to use the library? With what compiler?
Did you have any problems?
No. * How much effort did you put into your evaluation?
A glance? A quick reading? In-depth study?
I read the documentation, looked through the implementation and glanced at the tests. * Are you knowledgeable about the problem domain?
Yes.
And finally, every review should answer this question:
* Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion.
No. pj