Pavel, Overall, I do not consider the library ready for acceptance into Boost. My review follows: Jason's Singleton library would fill a clear niche, and the demand for a flexible implementation such as this is obviously high. I think this particular implementation has some novel ideas, particularly "multiton," which I know I've had to implement before, but have never seen anybody codify as a pattern per se. I also think that Jason had a good jumping-off point in starting with Andrei Alexandrescu's Loki::SingletonHolder. However I think Jason's work suffers from a number of problems, both conceptual and concrete. I'll rattle these off in no particular order: 1. Creator policy is redundant with the standard Allocator concept, I think. Though the standard Allocator concept has some subtle, tricky semantics, I think it's nevertheless worth using: that would permit interoperability with existing allocator implementations. 2. The auto_reset struct has some conceptual issues. The biggest of these is that the scope of the auto_reset object, if I understand it correctly, can exceed the lifetime of the variable it references. If such an object is strictly necessary, I believe it should own the object it references. Another minor nit: the template should be parametrized not only on the variable type, but also the reset value type. And lastly, the documentation contains implementation details in the interface. 3. The leaky_lifetime class is indicated as "not recommended for general use." I tend to disagree: in production code I've had experience with, it's been very appropriate to manually control the order of destruction of singleton resources, because there are often dependencies. Some of these issues can be mitigated by dependency_lifetime, which is a good contribution. But I think the use of leaky_lifetime shouldn't be explicitly discouraged. I think it would be more appropriate to name the class simply "unmanaged_lifetime". 4. I think multiton is a good idea, but its execution is incomplete. If it's in the library, my feeling is that it should implement the requirements of the standard Pair Associative Container fully. (I'd also suggest calling it singleton_map). 5. The polymorphic_pointer and polymorphic_reference helpers seem sketchy to me somehow. I don't understand their use. I'd like to see use cases in the documentation. 6. The exists(), is_null() and operator bool() methods fall prey to a well known C++ pitfall. Have a look at Bjorn Karlsson's article, "The Safe Bool Idiom": http://www.artima.com/cppsource/safebool.html. 7. I want to reiterate my overwhelming disagreement with both the necessity for, and the approach taken, by the pure virtual enable_singleton_creation method. This has been discussed at length in the thread rooted at http://aspn.activestate.com/ASPN/Mail/Message/2531666. 8. The docs are sorely lacking in detail. For example, the methods in the basic_singleton and singleton_ex interfaces are not enumerated anywhere, and none of the examples I saw show how to get an instance of a singleton. All that I felt was illustrated were various regurgitations of the "curiously recurring template pattern." Personally, this gives me the impression (justified or not) that Jason has been focusing more on the cleverness of his implementation than what it has to offer in terms of genuine novelty or general usefulness. Which brings me to my last point: 9. When implementing a Tree library has been discussed on the Boost mailing list, the biggest point that I feel was made in that discussion was (paraphrasing) "any idiot can implement a tree, but thinking about the discrete concepts that comprise the wide variety of trees, and about the concepts' interoperability with other existing concepts such as standard containers and algorithms, is a much harder task." I feel the same critique applies here. The library does offer a novel concept embodied by "multiton", but my comments about multiton's failure to implement Pair Associative Container and of the library's use of a creator policy rather than standard allocators, are examples of this. I feel that some deeper thought should be given to the concepts: all the rest is fluff. To address the standard questions, briefly: * What is your evaluation of the design? A good basic idea with some excellent, novel contributions, but some design problems (primarily inattention to concepts) * What is your evaluation of the implementation? Problematic (see above) * What is your evaluation of the documentation? Very incomplete. Interfaces are not completely described, some implementation detail remains in the interfaces, and use cases are lacking. * What is your evaluation of the potential usefulness of the library? Potentially very useful. I look forward to seeing some implementation of the singleton pattern make its way into Boost eventually. * Did you try to use the library? With what compiler? Did you have any problems? I did not use it. * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? Somewhere between a quick reading and an in-depth study. I focused on some areas and skimmed others. * Are you knowledgeable about the problem domain? Yes, reasonably knowledgeable. Just like the rest of us I've implemented enough singletons and "multitons" to know about the basics of design, and the relevant issues in production code. Again, to summarize, I think Jason's work shows promise, but needs a lot of work before I'd consider it acceptable. Regards, dr On 5/4/05, Pavel Vozenilek <pavel_vozenilek@hotmail.com> wrote:
The formal review of Singleton library written by Jason Hise starts today, May 5th, and lasts 10 days.