John Torjo wrote:
Today starts the formal Fast-Track review of the Boost.Utility/Singleton library.
What to include in Review Comments ==================================
Here are some questions you might want to answer in your review:
* What is your evaluation of the design?
Very strong. I like it! I've implemented CRTP Singleton templates a few times before -- highlighting the need for a Boost library. ;-) I'm pleased with the deluxe functionality: - singleton, mutexed_singleton and thread_specific_singleton - bypassing the requirement for friend declarations with the ctor taking boost::restricted - 'instance' as a static data member rather than a static member function - the lease mechanism - control of destruction order My one additional comment on design: people are very used to writing my_singleton::instance()->member. I like the coolness of writing my_singleton::instance->member instead, but I suspect I'll hear some grumbling from colleagues who keep getting reminded by compile errors. I wish it were possible to provide 'instance' as both a static data member and a static member function... But I'd be good with having this become the new convention.
* What is your evaluation of the implementation?
I didn't review the implementation.
* What is your evaluation of the documentation?
Very clear, though not entirely complete, as described below. The organization and style is excellent, and when polished, this documentation will be exemplary. I like the examples. Pointing out the ability to bind() instance and/or lease, and the semantics of each, is great. Also the multiple-inheritance example to graft Singleton behavior onto an existing class. I didn't fully understand the thrust of the example described as: "We might as well have a static facility use the Singleton internally, by using non-public inheritance". A few more words about that example would be useful; maybe a bit of the implementation of static void message(). It appears that the third template parameter SubsystemTag was a recent introduction. The documentation (especially the reference material) doesn't describe it enough, and often fails to mention it entirely. The first paragraph under http://torjo.com/tobias/index.html#boost_utility_singleton._singletons_and_d... seems to want to be a bullet list that got flowed instead. Very little is said about BOOST_SINGLETON_PLACEMENT and BOOST_SINGLETON_PLACEMENT_DECLARATION. The reference material mentions them without any explanation. The example http://torjo.com/tobias/index.html#boost_utility_singleton._singletons_and_d... appears to be inconsistent: shouldn't mylib_tag and library_tag be the same, one way or the other? Also, that example combines three different mechanisms: BOOST_SINGLETON_PLACEMENT, destroy_singletons() and SubsystemTag. More explanation of the role of each (why are we using that mechanism here?), and how they interact -- or remain orthogonal -- would clarify.
* What is your evaluation of the potential usefulness of the library?
Very. "If it didn't exist, we would have to invent it." ;-)
* Did you try to use the library? With what compiler? Did you have any problems?
I didn't try it.
* How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I read the documentation and cross-checked a couple of interface points (SubsystemTag) in the source code.
* Are you knowledgeable about the problem domain?
Yes, I've implemented the Singleton pattern a few times now.
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.
Yes! Thumbs up! Once the documentation is updated as mentioned above, this will be an excellent and much-needed addition to Boost.